feat(schemas): separate public from internal schema#29041
feat(schemas): separate public from internal schema#29041
Conversation
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
d706910 to
16892e6
Compare
Otherwise `ajv` rejects the value for not being a string.
- Avoid unnecessary `tsType`. - Avoid descriptions next to `$ref` (causes duplicate types). - Refine descriptions.
Compares build output between PR and base.
225c0dd to
fe10aa9
Compare
Same format as other two schema descriptions.
ddbeck
left a comment
There was a problem hiding this comment.
Partial review here, but I'm done for the day, so I'll leave it at that. There's plenty to chew on already though.
| - name: Checkout | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 |
There was a problem hiding this comment.
All of the name: additions are pretty pointless here. Not sure why any are in this PR.
| } | ||
| ] | ||
| ], | ||
| "typescript.tsdk": "node_modules/typescript/lib" |
| * - Replaces `browser: { version_added: "mirror" }` with `browser: "mirror"` | ||
| * - Wraps `browser: false` with `browser: `{ version_added: false }` | ||
| * @param {CompatStatement} compat The compat statement to fix | ||
| * @param {Pick<InternalCompatStatement, "support">} compat The compat statement to fix |
There was a problem hiding this comment.
| * @param {Pick<InternalCompatStatement, "support">} compat The compat statement to fix | |
| * @param {Pick<InternalCompatStatement, "support">} compat The support block to fix |
| import { fixStatusValue } from './status.js'; | ||
|
|
||
| /** @type {{ name: string; input: TestValue; output: TestValue }[]} */ | ||
| /** @type {{ name: string; input: *; output: * }[]} */ |
There was a problem hiding this comment.
Eh, why go any here? Why not use the new types?
| export default await load(...dataFolders); | ||
| /** @type {InternalCompatData} */ | ||
| const bcd = await load(...dataFolders); | ||
|
|
||
| export default bcd; | ||
|
|
||
| /** @type {Browsers} */ | ||
| export const browsers = bcd.browsers; |
There was a problem hiding this comment.
I don't understand this change. Can you explain this?
| }, | ||
| "flags": { | ||
| "type": "array", | ||
| "description": "An optional array of objects describing flags that must be configured for this browser to support this feature.", |
There was a problem hiding this comment.
| "description": "An optional array of objects describing flags that must be configured for this browser to support this feature.", | |
| "description": "Array of objects describing flags that must be configured for this browser to support this feature", |
| } | ||
| } | ||
| ], | ||
| "description": "An optional changeset/commit URL for the revision which implemented the feature in the source code, or the URL to the bug tracking the implementation, for the associated browser.", |
There was a problem hiding this comment.
| "description": "An optional changeset/commit URL for the revision which implemented the feature in the source code, or the URL to the bug tracking the implementation, for the associated browser.", | |
| "description": "A version control URL for the revision which implemented the feature or a bug URL for tracking implementation", |
| "errorMessage": { | ||
| "pattern": "impl_url must be a link to a browser commit or bug URL. URLs must be in shortened form (ex. bugs.chromium.org -> crbug.com). Note: `npm run fix` may resolve these issues." | ||
| } |
There was a problem hiding this comment.
I don't believe this is a JSON Schema field and I don't think we should bother public consumers with text about internal tools.
| }, | ||
| "partial_implementation": { | ||
| "const": true, | ||
| "description": "A boolean value indicating whether or not the implementation of the sub-feature deviates from the specification in a way that may cause compatibility problems. It defaults to false (no interoperability problems expected). If set to true, it is recommended that you add a note explaining how it diverges from the standard (such as that it implements an old version of the standard, for example)." |
There was a problem hiding this comment.
This is very long. Another approach here (though one that requires a bit of work elsewhere) would be to break this up into title and description.
Other things:
- It shouldn't default to anything. It should be optional and never any value but
true - Avoid putting internal guidance into the public schema
| "description": "A boolean value indicating whether or not the implementation of the sub-feature deviates from the specification in a way that may cause compatibility problems. It defaults to false (no interoperability problems expected). If set to true, it is recommended that you add a note explaining how it diverges from the standard (such as that it implements an old version of the standard, for example)." | |
| "description": "Set if implementation of the subfeature is notably incomplete or broken." |
| "dependencies": { | ||
| "partial_implementation": { | ||
| "if": { | ||
| "properties": { "partial_implementation": { "const": true } } | ||
| }, | ||
| "then": { "required": ["notes"] } | ||
| }, | ||
| "version_removed": { | ||
| "required": ["version_last"] | ||
| } |
There was a problem hiding this comment.
This is not real JSON Schema. Why did you write this? Who did you expect to consume this data?
Summary
Adds a public schema for
data.json, separate from the internal browser/compat file schemas.Test results and supporting details
There are two changes that are strictly speaking breaking:
CompatStatement.source_fileproperty is now required.BrowserStatement.upstreamproperty is narrowed down toUpstreamBrowserName, a subset ofBrowserName.Before
After
diff
Related issues
Fixes #29059.