enhance(build): validate built data.json against schemas#27685
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.
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.
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.
How about checking process.env.CI and failing if we're in CI?
There was a problem hiding this comment.
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.
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.
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. |
|
|
||
| for (const [key, value] of Object.entries(data)) { | ||
| if (key === '__meta') { | ||
| // Not covered by the schema. |
| const writeData = async () => { | ||
| const dest = new URL('data.json', targetdir); | ||
| const data = await createDataBundle(); | ||
| validate(data); |
There was a problem hiding this comment.
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.