-
Notifications
You must be signed in to change notification settings - Fork 175
Demonstrate JSON Schema as source of truth for web-features data #2990
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
Demonstrate JSON Schema as source of truth for web-features data #2990
Conversation
@@ -0,0 +1,208 @@ | |||
/** |
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.
newtypes.ts
Outdated
export interface WebFeaturesData | ||
extends Pick<QuicktypeWebFeaturesData, "browsers" | "groups" | "snapshots"> { | ||
features: { [key: string]: FeatureData }; | ||
} | ||
|
||
export type FeatureData = Required< | ||
Pick< | ||
QuicktypeMonolithicFeatureData, | ||
"description_html" | "description" | "name" | "spec" | "status" | ||
> | ||
> & | ||
Partial< | ||
Pick< | ||
QuicktypeMonolithicFeatureData, | ||
"caniuse" | "compat_features" | "discouraged" | ||
> | ||
>; |
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.
This is a preview of what's coming when we add redirects. TypeScript and JSON Schema's type systems aren't strictly equivalent, so quicktype will generate correct but overbroad types. In this module, I'll pluck out some nuances (and badly generated names) that quicktype doesn't handle very well and clean them up by extending the generated types. While it's a bit weird looking, the benefit is that we can't completely diverge from the underlying JSON 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.
Do the TypeScript error messages if you get types wrong end up looking completely bizarre with this layering?
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.
They're not quite as nice as plain, from-scratch types but they're probably above-average in terms of informativeness in type error messages. I've seen some { … } & { … } & { … } & …
monstrosities and this is nicer than that.
newtypes.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const t1: FeatureData = { | ||
name: "Test", | ||
description: "Hi", | ||
description_html: "Hi", | ||
spec: "", | ||
status: { | ||
baseline: false, | ||
support: {}, | ||
}, | ||
}; |
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.
If we like, we can even "test" some properties of our types—like I do here demonstrating a FeatureData
object without any of the optional fields—by instantiating some objects and letting the typechecker complain if we get it wrong.
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 seems like a useful smoke test. I guess that when we load the JSON and interpret it as a TypeScript type, there's no checking going on that we could rely on? That would be helpful, but I suspect it's not a thing.
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.
We could do this, but I'd prefer to avoid it on the initial implementation. Quicktype can generate zod runtime type check code, but it'd be a new dependency that I don't feel especially ready to embrace just yet.
schemas/new.schema.json
Outdated
@@ -0,0 +1,302 @@ | |||
{ |
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.
And here is the JSON Schema, intended for a human (me) to work with, instead of the generated JSON Schema. Mostly, things are ordered in a more readable way.
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 like it! By now I can almost read JSON Schema, and I find this quite readable.
7c9c780
to
9846b90
Compare
I think we should do it! I'd rather have ugly TypeScript definitions than ugly JSON Schema. |
Thank you for the review, @jcscottiii. Thanks to your review I was able to find some other constraints that weren't in the new schema file. This also prompted me to discover some oddities in the rolled-up TypeScript types in npm package — I'm still working on fixing that part. |
I tried out the latest updates and it looks really good now! Thanks |
packages/web-features/package.json
Outdated
"types.d.ts", | ||
"types.quicktype.d.ts", | ||
"index.js", | ||
"data.json", | ||
"data.schema.json" | ||
], | ||
"scripts": { | ||
"prepare": "tsc && rm types.js && tsup ./index.ts --dts-only --format=esm --out-dir=." | ||
"prepare": "tsc && rm types.js && rm types.quicktype.js" | ||
}, | ||
"devDependencies": { | ||
"@types/node": "^24.0.7", | ||
"tsup": "^8.5.0", | ||
"typescript": "^5.8.3" |
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.
One thing I discovered in following up @foolip's #2990 (comment) question about type errors for consumers:
tsup
adds another layer of name-mangling, on top of the layer given by converting JSON Schema to TypeScript (via quicktype
). For example, as originally configured, FeatureData
is shown as extending FeatureData$1
. Ugly!
To fix this, I just let plain TypeScript generate the type declaration files. It's slightly less tidy — we have to include the types.d.ts
and types.quicktype.d.ts
files — but it's a very tiny hit for consumers (two additional small files and two development-time file reads for TypeScript consumers) and a nice boon for us (one fewer dependency and one less layer of indirection).
This also makes it a lot easier for us to do some other things in the future (e.g., adding util functions or making this a "normal" monorepo where we're doing less magic with the web-features
package).
Commit 6e383ff LGTM, less code and still works! |
/** | ||
* caniuse.com identifier(s) | ||
*/ | ||
caniuse?: string[] | string; |
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 don't want to delay v3, but how quickly could we decide that the built output will always be an array instead of string-or-array? Here and for a bunch of things below.
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 this PR, I tried my best to avoid changing anything material about the existing JSON schema, just flip around what we're generating (that is JSON schema → TypeScript, instead of the reverse—it's not exactly equivalent, which is why I haven't pushed to merge this sooner than v3). I expect we'll merge #3184 to fixup all the string-or-array types as part of v3. To the best of my knowledge, there's no opposition to landing that PR.
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.
Alright, excellent!
/** | ||
* Whether the feature is Baseline (low substatus), Baseline (high substatus), or not (false) | ||
*/ | ||
baseline: boolean | BaselineEnum; |
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.
Is it possible to put false
here to rule out true
as a possible value?
baseline: boolean | BaselineEnum; | |
baseline: false | BaselineEnum; |
Same change below if so.
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.
* Browser versions that most-recently introduced the feature | ||
*/ | ||
support: Support; | ||
[property: string]: any; |
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.
What does this line do?
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.
This whole file is generated by Quicktype. This line is a bug (or limitation, not sure) of Quicktype: it doesn't seem to be able to determine whether the Status
object might have additional properties (or what their types might be). The types.ts
file corrects this with the Pick<…>
types, so that these arbitrary keys are stripped from the exported types.
/** Whether developers are formally discouraged from using this feature */ | ||
discouraged?: Discouraged; | ||
export interface WebFeaturesData | ||
extends Pick<QuicktypeWebFeaturesData, "browsers" | "groups" | "snapshots"> { |
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.
Just about here is where I no longer understand what I'm reading and will just trust that this is all great. It sure looks like TypeScript. If you're really unsure about anything I can try harder to understand.
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 the interest of completeness and posterity, I'll explain this a bit better:
Here, I'm making the types nicer than what Quicktype generates by "picking" good stuff out of the schema, but partly modifying the generated type for the features["…"]
objects (FeatureData
, below). This isn't strictly required now, but will be when we add redirects (in a follow up PR).
Anyway, by doing it this way instead of writing wholly custom types, we preserve the link between JSON Schema and TypeScript.
Co-authored-by: Philip Jägenstedt <[email protected]>
…| false` (continued)
* Demonstrate JSON Schema as source of truth for web-features data (#2990) * Add move and split redirects to the schema (#3000) * Add guidelines for moving and splitting features (#3180) * Add consumer docs for moved and split features (#3181) * Change schema `string | string[]` properties to `string[]` (#3184) * Fix missing `group` and `snapshot` keys from convenience types Co-authored-by: James C Scott III <[email protected]> Co-authored-by: Philip Jägenstedt <[email protected]> Co-authored-by: Patrick Brosset <[email protected]> Co-authored-by: szepeviktor <[email protected]> Co-authored-by: LeoMcA <[email protected]> Co-authored-by: foolip <[email protected]>
This PR demonstrates switching from generating JSON Schema to authoring a JSON Schema. This is a prerequisite for #91 and fixes #2722. I'll self-review to point out some interesting things.