-
Notifications
You must be signed in to change notification settings - Fork 2.4k
enhance(build): validate built data.json against schemas
#27685
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
Conversation
data.json against schemas
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
| const writeData = async () => { | ||
| const dest = new URL('data.json', targetdir); | ||
| const data = await createDataBundle(); | ||
| validate(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
If validation fails, the validation errors are logged via console.error, but we don't bail out, to avoid disruption for contributors in the unlikely event that this ever happens on main. In either case, these errors won't get unnoticed, because they will appear on every npm install (via the prepare script).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that a PR that breaks schema validation won't fail the build? I can kinda see the case for never failing during the prepare script, but it'd be nice if there were a test that does fail. I won't block on this though, as it would be a step up from what we have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about checking process.env.CI and failing if we're in CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would help for local development, but not pull request checks. Maybe check if GITHUB_REF is refs/heads/main and, if true, convert errors to warnings, else do nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pull request checks, we always have CI=true in the environment, so we would notice it in a PR before merging. (In fact, the PR check would fail, preventing a merge.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's tackle this afterwards.
FWIW, I raised this as a concern in #17574 and got shot down on doing anything about it. |
FWIW, here's the meta schema (isolated, like the other two): {
"$schema": "http://json-schema.org/schema",
"definitions": {
"meta_block": {
"type": "object",
"properties": {
"timestamp": {
"type": "string",
"format": "date-time"
},
"version": {
"type": "string"
}
},
"required": ["timestamp", "version"],
"additionalProperties": false
}
},
"properties": {
"__meta": {
"$ref": "#/definitions/meta_block"
}
},
"title": "MetaData",
"type": "object",
"required": ["__meta"],
"additionalProperties": false,
"maxProperties": 1,
"minProperties": 1
}Unfortunately, ajv gives me an error when validating the data: That error doesn't make sense, but given this is not a priority, I would declare the scope of this PR to validate the built data against the existing schemas. |
b884975 to
c046688
Compare
The built data.json contains multiple browsers.
c046688 to
fabb4f2
Compare
|
@queengooborg If we can get this in, we can split up the schema into internal/public, and validate separately. Currently, it's a mix of both. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the schema change 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the code. Thank you!
|
|
||
| for (const [key, value] of Object.entries(data)) { | ||
| if (key === '__meta') { | ||
| // Not covered by the schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also 😒 #17574
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also: #27685 (comment)
| const writeData = async () => { | ||
| const dest = new URL('data.json', targetdir); | ||
| const data = await createDataBundle(); | ||
| validate(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that a PR that breaks schema validation won't fail the build? I can kinda see the case for never failing during the prepare script, but it'd be nice if there were a test that does fail. I won't block on this though, as it would be a step up from what we have.
Summary
data.jsonagainst the schemas.maxPropertiesrequirement for thebrowsersobject from the browsers schema.Test results and supporting details
This revealed two minor issues:
browsers. This requirement only applies to the individualbrowser/*.jsonfiles, not the builtdata.json, so this requirement was removed.__metaobject in the builtdata.jsonis not currently covered by the schema, so we cannot validate it.Related issues
Fixes #27564.