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.
Move extra types to `types/index.d.ts`.
|
This pull request has merge conflicts that must be resolved before it can be merged. |
|
This is still a bit confused/confusing:
|
This is a dilemma, but the resolution to
This is pre-existing, and I don't think it has actually caused any confusion. The
These are now fixed.
It seems that the TypeScript errors in |
LeoMcA
left a comment
There was a problem hiding this comment.
Ok, nice! I built locally and installed in fred and typescript was happy.
So we don't rely on the web-features type fixes PR being merged, and so we can ensure type breakages aren't introduced in the future, it would be nice to drop a tsconfig.json like this into types/ and build/:
{
"extends": "../tsconfig.json",
"compilerOptions": {
"skipLibCheck": false,
}
}
I tested locally, with deliberate changes to break those files, and it surfaced them when running with cd types && npx tsc && cd ../build && npx tsc.
I added it to I hope the web-features PR gets accepted, and then we can disable |
I think it most needs to be there: those are the types we ship to consumers, and those are the types it's most essential to ensure are correct, and the ones least likely to be noticed are broken while working on bcd. Is using |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
You made good points, and convinced me. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
There was a problem hiding this comment.
It looks like the npm run build command deletes everything in this folder, so ends up deleting the tsconfig.json
I also can't see where we run typescript to check the types are valid - we should do this against this tsconfig as some sort of pre-build/release test - it would protect against shipping bad types to consumers
ddbeck
left a comment
There was a problem hiding this comment.
Feeling good about the consumer-facing bits of this (I'm kinda deferring to Leo on the types issues—I think he's better situated to review that). Some small stuff noted in line comments. Thank you!
|
|
||
| - An optional `spec_url` property as a URL or an array of URLs, each of which is for a specific part of a specification in which this feature is defined. | ||
| Each URL must either contain a fragment identifier (e.g. `https://tc39.es/proposal-promise-allSettled/#sec-promise.allsettled`), or else must match the regular-expression pattern `^https://registry.khronos.org/webgl/extensions/[^/]+/` (e.g. `https://registry.khronos.org/webgl/extensions/ANGLE_instanced_arrays/`). | ||
| Each URL must either contain a fragment identifier (e.g. `https://tc39.es/proposal-promise-allSettled/#sec-promise.allsettled`), or else must match the regular-expression pattern `^https://registry.khronos.org/webgl/extensions/[^/]+/` (e.g. `https://registry.khronos.org/webgl/extensions/ANGLE_instanced_arrays/`) or `^https://github.com/WebAssembly/.+` for WebAssembly specs. |
There was a problem hiding this comment.
In that case, I would still suggest omitting the exact patterns, to minimize the risk of the two becoming mismatched. Something like this:
| Each URL must either contain a fragment identifier (e.g. `https://tc39.es/proposal-promise-allSettled/#sec-promise.allsettled`), or else must match the regular-expression pattern `^https://registry.khronos.org/webgl/extensions/[^/]+/` (e.g. `https://registry.khronos.org/webgl/extensions/ANGLE_instanced_arrays/`) or `^https://github.com/WebAssembly/.+` for WebAssembly specs. | |
| Each URL must contain a fragment identifier (e.g. `https://tc39.es/proposal-promise-allSettled/#sec-promise.allsettled`) or match one of the regular-expression pattern given in the JSON Schema file. |
| "properties": { | ||
| "experimental": { | ||
| "type": "boolean", | ||
| "deprecated": true, |
There was a problem hiding this comment.
I've noticed that my editor now shows an underline on deprecated when editing BCD JSON. If this is the internal-only schema now, we can remove this so that contributors know the data is nonetheless required and unavoidable.
| "deprecated": true, |
| "type": "boolean", | ||
| "deprecated": true, | ||
| "description": "(This property is deprecated. Prefer using more well-defined stability calculations, such as Baseline, instead.) A boolean value. Usually, this value is true for single-implementer features and false for multiple-implementer features or single-implementer features that are not expected to change." | ||
| "description": "(This property is deprecated. Prefer using more well-defined stability calculations, such as Baseline, instead.) Usually, this value is true for single-implementer features and false for multiple-implementer features or single-implementer features that are not expected to change." |
There was a problem hiding this comment.
| "description": "(This property is deprecated. Prefer using more well-defined stability calculations, such as Baseline, instead.) Usually, this value is true for single-implementer features and false for multiple-implementer features or single-implementer features that are not expected to change." | |
| "description": "(This property is deprecated in the public facing schema, but we still have to set the data internally.) Usually, this value is true for single-implementer features and false for multiple-implementer features or single-implementer features that are not expected to change." |
There was a problem hiding this comment.
avoid Latin abbreviations
Examples?
It looks like the only remaining ones are "e.g."; replace with "for example, [whatever]".
On the "key" discussion, I like Leo's suggestion of "browser ID." 👍
| "dependencies": { | ||
| "partial_implementation": { | ||
| "if": { | ||
| "properties": { "partial_implementation": { "const": true } } | ||
| }, | ||
| "then": { "required": ["notes"] } | ||
| }, | ||
| "version_removed": { | ||
| "required": ["version_last"] | ||
| } |
There was a problem hiding this comment.
Ahh, OK. They're ajv extensions. They're not in the https://json-schema.org/ spec. It's OK to leave this in.
(When I looked this up independently, I could only find dependencies as a React-ism on top of JSON Schema.)
| } | ||
| } | ||
| ], | ||
| "description": "URL(s) for specific parts of a specification in which this feature is defined. Each URL must contain a fragment identifier." |
There was a problem hiding this comment.
It's nice that we include the fragments, but I think this is an editorial choice rather than a guarantee that we should explicitly make to consumers.
| "description": "URL(s) for specific parts of a specification in which this feature is defined. Each URL must contain a fragment identifier." | |
| "description": "URL(s) for specific parts of a specification in which this feature is defined." |
| "$ref": "#/definitions/version_value" | ||
| }, | ||
| "version_removed": { | ||
| "description": "Browser version that removed this feature.", |
There was a problem hiding this comment.
Something I've noticed on this pass is that we use (in both the documentation and schema itself) "subfeature" and "feature" more or less interchangeably. Since the schema is the same for top-level features as subfeatures, I think it would make sense to s/subfeature/feature/g.
|
|
||
| #### `version_last` | ||
|
|
||
| A string indicating the last browser version that supported the feature. This property is always present when `version_removed` is present. For example, if a feature was removed in version 30, `version_last` will be `"29"`. |
There was a problem hiding this comment.
optional: it might be nice to mention that version numbers are sometimes non-sequential (e.g., that version_removed is not always version_removed - 1).
Summary
Adds a public schema for
data.json, separate from the internal browser/compat file schemas.Also refines the schema descriptions based on Daniel's feedback.
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.