Skip to content

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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ddbeck
Copy link
Collaborator

@ddbeck ddbeck commented May 21, 2025

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.


This NOT ready to merge. Don't merge it! If we want to go down this route, then there are a few tasks remaining to get this into a mergeable state:

  • Remove old schema
  • Remove old generation
  • Add a test that the checked-in quicktype generated types file matches what we'd expect to be generated from JSON Schema (much like we do for the types → JSON Schema process today)

@github-actions github-actions bot added the tools and infrastructure Project internal tooling, such as linters, GitHub Actions, or repo settings label May 21, 2025
new.quicktype.ts Outdated
@@ -0,0 +1,208 @@
/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is generated by quicktype from the new, hand-authored JSON Schema. The generation is OK for now but if we add something like redirects as described in #91, it has a few inconveniences. We're going to insulate ourselves from quicktype with the next TS module.

newtypes.ts Outdated
Comment on lines 33 to 49
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"
>
>;
Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Comment on lines 51 to 61
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const t1: FeatureData = {
name: "Test",
description: "Hi",
description_html: "Hi",
spec: "",
status: {
baseline: false,
support: {},
},
};
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@@ -0,0 +1,302 @@
{
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

@ddbeck ddbeck requested a review from foolip May 21, 2025 16:34
@ddbeck ddbeck force-pushed the 91-jsonschema-source-of-truth branch from 7c9c780 to 9846b90 Compare May 22, 2025 07:09
@foolip
Copy link
Collaborator

foolip commented May 22, 2025

I think we should do it! I'd rather have ugly TypeScript definitions than ugly JSON Schema.

@ddbeck ddbeck added major version required This PR requires a minor version semver release (vX+1.0.0) package:web-features labels May 26, 2025
@ddbeck ddbeck added this to the web-features v3.0 milestone May 26, 2025
"$ref": "#/definitions/URL"
}
}
},
Copy link
Contributor

@jcscottiii jcscottiii Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: name and spec were previously marked as required. Now they are optional. Not sure if this was intentional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b2b6762.

],
"type": "object"
"description": "Whether a feature is considered a \"Baseline\" web platform feature and when it achieved that status",
"$ref": "#/definitions/StatusHeadline"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment here. baseline and support were previously required. But I think they should stay required. (Probably in StatusHeadline)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b2b6762.

"status"
],
"type": "object"
"required": ["name", "description", "description_html", "spec"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is status no longer required?

Suggested change
"required": ["name", "description", "description_html", "spec"]
"required": ["name", "description", "description_html", "spec", "status"]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b2b6762.

@ddbeck
Copy link
Collaborator Author

ddbeck commented Jun 11, 2025

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.

@jcscottiii
Copy link
Contributor

I tried out the latest updates and it looks really good now! Thanks

Comment on lines +21 to 32
"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"
Copy link
Collaborator Author

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).

@foolip
Copy link
Collaborator

foolip commented Jul 7, 2025

Commit 6e383ff LGTM, less code and still works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major version required This PR requires a minor version semver release (vX+1.0.0) package:web-features tools and infrastructure Project internal tooling, such as linters, GitHub Actions, or repo settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rethink JSON Schema generation
3 participants