Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: move InitialOptions to JSON schema #14776

Merged
merged 4 commits into from
Dec 27, 2023
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 24, 2023

Summary

Part of #11963. I think with this, we can look into writing the resulting schema to disk and somehow publish it. Maybe as part of the website?

@sinclairzx81 FYI 🙂 Any and all suggestions here very much welcome 👍

Test plan

Green CI

Copy link

netlify bot commented Dec 24, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 76c7d0e
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/658beee6a1be010008a0d91b
😎 Deploy Preview https://deploy-preview-14776--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Type.Object({
global: RawCoverageThresholdValue,
}),
// TODO: is there a better way of doing index type?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is currently

type CoverageThreshold = {
  [path: string]: CoverageThresholdValue;
  global: CoverageThresholdValue;
};

I don't think my current approach is the correct "translation" of that, but it seems to work in practice

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SimenB!

The above type would represent the TS data structure (both validation and inference). However the allOf representation could be a consideration if publishing (as it would require downstream tooling to be able to enumerate properties of the intersection)

There is another way to represent this type, which would ensure it remains an "object" type.

const RawCoverageThreshold = Type.Object({
  global: RawCoverageThresholdValue,              // global: RawCoverageThresholdValue
}, {
  additionalProperties: RawCoverageThresholdValue // [path: string]: RawCoverageThresholdValue 
})

TypeBox does not support auto inference for additionalProperties of TSchema, But it is possible to wrap in Unsafe which allows you to specify the correct inference type.

const RawCoverageThresholdBase = Type.Object({
  global: RawCoverageThresholdValue,
}, {
  additionalProperties: RawCoverageThresholdValue
})

const RawCoverageThreshold = Type.Unsafe<{
  global: Static<typeof RawCoverageThresholdValue>,
  [path: string]: Static<typeof RawCoverageThresholdValue>
}>(RawCoverageThresholdBase)

It's also possible to pass Unsafe arbitrary JSON Schema + Type if you need specific representations outside the ones provided by TypeBox.

Hope this helps! :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does, thank you!

'\n\nThe default is `[]`, meaning all APIs are faked.',
default: [],
}),
now: Type.Union([Type.Integer({minimum: 0}), Type.Date()], {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would recommend removing Type.Date() as this type is a JS extension type in TB (so would be unknown to Ajv). But could represent as the following.

Type.Object({
  // ...
  now: Type.Unsafe<number|Date>(Type.Integer({minimum: 0}))
})

This would ensure that now be integer only, but with the type suggesting a potential for the property to converted into a Date object. Have a quick read over the TB Transform feature as this may be able to automatically transform values like this on value load, something that could potentially be run post successful Ajv validate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already do work to make the type different from config input and usage at runtime:

type FakeTimers = GlobalFakeTimersConfig &
(
| (FakeTimersConfig & {
now?: Exclude<FakeTimersConfig['now'], Date>;
})
| LegacyFakeTimersConfig
);

And yeah, plugging the schema and its defaults with transformers into our normalize function is a long term plan 🙂

@SimenB SimenB enabled auto-merge (squash) December 27, 2023 09:32
@SimenB SimenB merged commit 896573b into jestjs:main Dec 27, 2023
73 checks passed
@SimenB SimenB deleted the more-typebox branch December 27, 2023 21:23
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants