Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Publish groups and snapshots #1060

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

foolip
Copy link
Collaborator

@foolip foolip commented May 8, 2024

No description provided.

@captainbrosset
Copy link
Contributor

Discussed in today's WebDX call (notes).

Philip: the bigger topic here is coming up with guidelines for groups. While groups are easy for like 50% of features, it gets harder for other features. Is grid animation part of animations, layout? Which group should structuredCloned be in?
Giving everything a group might get messy. Tags might help.

Patrick: MDN is very interested in groups, for information architecture purposes. We should revive this discussion with MDN and OWD.

@foolip
Copy link
Collaborator Author

foolip commented Jun 24, 2024

The key question blocking this for me is whether we should change the default export of the package, or just add named exports for groups and snapshots. In either case we can make the following work:

import { features, groups } from 'web-features';

But what should the default export be?

import thing from 'web-features';

What is thing here? Is it an object with features, groups, and snapshots as keys, or is it still the same thing as currently, the features object?

Both are easy to do, but I'm not sure what's typical for JS modules.

@ddbeck
Copy link
Collaborator

ddbeck commented Jun 25, 2024

My preference (though not strongly held) would be to not have a default export at all. If we change what we're exporting, it'll break in a more transparent way for consumers. I'd much rather get a SyntaxError: The requested module 'web-features' does not provide an export named 'default' instead of undefined from a feature lookup, if I'm upgrading from this.

This also makes it less risky for us to embellish the package in the future. For instance, if we add some utils functions, then tree shaking ought to place a lesser burden on consumers that don't need that code. I don't think the same would be true for a default export.

@foolip foolip force-pushed the publish-groups-snapshots branch 2 times, most recently from a1fa8f7 to 1c58c67 Compare July 2, 2024 16:02
@foolip foolip marked this pull request as ready for review July 2, 2024 16:02
@foolip
Copy link
Collaborator Author

foolip commented Jul 2, 2024

@ddbeck I've removed the default export so we only have named exports now.

The remaining thing I need feedback on is the schema. It's still split into defs.schema.json and features.schema.json, but features.schema.json is no longer a great name. I can't quite tell why we have two files. What would need to be done to have a single self-contained schema file that we could then rename?

@foolip
Copy link
Collaborator Author

foolip commented Jul 2, 2024

I tinkered and it seems like defs.schema.json already is standalone. I've pushed a commit to remove features.schema.json.

Comment on lines +216 to +220
"additionalProperties": {
"$ref": "#/definitions/FeatureData"
},
"description": "Feature identifiers and data",
"type": "object"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing you'll lose here is the patternProperties constraint from features.schema.json:

  "patternProperties": {
    "^[a-z0-9-]*$": {
      "$ref": "defs#/definitions/FeatureData"
    }
  }

That said, if I rename a file to use underscores, ajv seems to not catch the break with the schema. So maybe this doesn't matter, or never has? 🤷

See #44 (comment) for background

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to find a way to express this with TypeScript but failed. It may be possible with https://www.typescriptlang.org/docs/handbook/2/template-literal-types.html but it wouldn't be pretty, and I can't imagine it would get turned into a pretty RegEx in JSON schema.

Maybe #1317 instead?

foolip added a commit to foolip/web-features that referenced this pull request Jul 4, 2024
ddbeck pushed a commit that referenced this pull request Jul 8, 2024
@foolip foolip merged commit 6d02245 into web-platform-dx:main Jul 10, 2024
3 checks passed
@foolip foolip deleted the publish-groups-snapshots branch July 10, 2024 09:59
@captainbrosset
Copy link
Contributor

Groups and snapshots are published! Yay!

@captainbrosset
Copy link
Contributor

You can see a very simple visualization of groups and features that have parent groups at https://web-platform-dx.github.io/web-features-explorer/groups/.

@foolip
Copy link
Collaborator Author

foolip commented Jul 10, 2024

That was quick, thank you @captainbrosset! This view immediately makes some odd things clear, like the group for Grid while Flexbox doesn't have one. We'll need guidelines for groups as well :)

jcscottiii added a commit to GoogleChrome/webstatus.dev that referenced this pull request Jul 16, 2024
The Web Features repo changed the structure of the data.json in this PR: web-platform-dx/web-features#1060

Instead of being a map of features, it is now an object with three top level keys:
- features
- groups
- snapshots

This is a breaking change and has caused us to start ingesting features as if they had IDs: features, groups, snapshots

This commit fixes that by updating the defs.schema.json to be the latest version (v0.10.0). This file comes from the web features repo.

After auto-generating the code for that schema file, this commit adjusts the code to use it.

We previously had v0.6.0 of the schema. And since then, some optional fields have become required fields. So you will see the
removal of some nil checks (since now the field is not nullable).

Change-Id: I9861421bc53ec8966eee80c33742d2a7e94d744c
github-merge-queue bot pushed a commit to GoogleChrome/webstatus.dev that referenced this pull request Jul 16, 2024
The Web Features repo changed the structure of the data.json in this PR: web-platform-dx/web-features#1060

Instead of being a map of features, it is now an object with three top level keys:
- features
- groups
- snapshots

This is a breaking change and has caused us to start ingesting features as if they had IDs: features, groups, snapshots

This commit fixes that by updating the defs.schema.json to be the latest version (v0.10.0). This file comes from the web features repo.

After auto-generating the code for that schema file, this commit adjusts the code to use it.

We previously had v0.6.0 of the schema. And since then, some optional fields have become required fields. So you will see the
removal of some nil checks (since now the field is not nullable).

Change-Id: I9861421bc53ec8966eee80c33742d2a7e94d744c
@foolip foolip mentioned this pull request Jul 17, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants