Skip to content
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

feat(schemas/browsers)!: merge accepts_{flags,webextension} fields into accepts array #25825

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Jan 31, 2025

Summary

Merges the accepts_flags and accepts_webextensions fields in the browser schema into a single accepts object with boolean properties flags and webextensions.

Important

This is a breaking change, as we expose these fields in the built data.json.

Test results and supporting details

Prepares for #25804, which adds webdriver-bidi.

Related issues

@github-actions github-actions bot added schema Isses or pull requests regarding the JSON schema files used in this project. infra Infrastructure issues (npm, GitHub Actions, releases) of this project docs Issues or pull requests regarding the documentation of this project. linter Issues or pull requests regarding the tests / linter of the JSON files. data:browsers Data about browsers (versions, release dates, etc). This data is used for validation. scripts Issues or pull requests regarding the scripts in scripts/. labels Jan 31, 2025
@caugner caugner changed the title [Schema] Merge browser accepts_* fields into accepts array [Schema] Merge browser accepts_{flags,webextension} fields into accepts array Jan 31, 2025
@github-actions github-actions bot added the size:l [PR only] 101-1000 LoC changed label Jan 31, 2025
@caugner caugner added the semver-major-bump A change that is potentially breaking for consumers label Jan 31, 2025
@caugner caugner changed the title [Schema] Merge browser accepts_{flags,webextension} fields into accepts array feat(schemas/browsers)!: merge accepts_{flags,webextension} fields into accepts array Jan 31, 2025
Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Considering that we're adding more flags for featuresets browsers allow, this makes sense.

Small nit: perhaps it would make more sense to make this a dictionary instead, so that we can add version numbers for when a featureset was introduced?

@caugner caugner added this to the BCD 6.0 milestone Feb 18, 2025
@caugner
Copy link
Contributor Author

caugner commented Feb 24, 2025

Small nit: perhaps it would make more sense to make this a dictionary instead, so that we can add version numbers for when a featureset was introduced?

@queengooborg I'm open to discussing the use case behind making this a dictionary, but I would prefer to start with this basic approach, to avoid scope creep.

@queengooborg
Copy link
Contributor

queengooborg commented Feb 26, 2025

I think that using a dictionary instead of an array of strings would have the following benefits:

  • Easier to migrate codebase
    • Consumers would only have to change a small bit of code: from if (browser.accepts_flags) to if (browser.accepts.flags) (instead of browser.accepts.includes('flags'))
  • Opens the possibility for adding version numbers (mentioned above)
    • This is something that I have been thinking about implementing in the future. The motivation for this is to enforce version number consistency for specific categories for when a feature category was introduced, and I've been thinking about extending it to other categories like webassembly
    • This would also mean that we don't have to release a new major version if we convert the schema to this structure
  • Allows for retaining TypeScript documentation
    • If we continue to use a dictionary, we can add TypeScript/schema comments that describe what the property means in better detail

@caugner caugner force-pushed the replace-browser-accepts-fields branch from 165cbd2 to e6c9b80 Compare February 26, 2025 17:44
@caugner
Copy link
Contributor Author

caugner commented Feb 26, 2025

I think that using a dictionary instead of an array of strings would have the following benefits:

Done in a7ede72.

@caugner caugner force-pushed the replace-browser-accepts-fields branch from e6c9b80 to 27fcdd6 Compare February 26, 2025 17:48
@caugner caugner force-pushed the replace-browser-accepts-fields branch from 27fcdd6 to 30806bc Compare February 26, 2025 17:48
@caugner caugner force-pushed the replace-browser-accepts-fields branch from 30806bc to a7ede72 Compare February 26, 2025 17:50
@caugner caugner requested a review from queengooborg February 26, 2025 20:29

An optional boolean indicating whether the browser supports flags. If it is set to `false`, flag data will not be allowed for that browser.
An optional object indicating which additional features the browser supports. Possible properties are:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
An optional object indicating which additional features the browser supports. Possible properties are:
An object indicating which additional features the browser supports. Possible properties are:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:browsers Data about browsers (versions, release dates, etc). This data is used for validation. docs Issues or pull requests regarding the documentation of this project. infra Infrastructure issues (npm, GitHub Actions, releases) of this project linter Issues or pull requests regarding the tests / linter of the JSON files. schema Isses or pull requests regarding the JSON schema files used in this project. scripts Issues or pull requests regarding the scripts in scripts/. semver-major-bump A change that is potentially breaking for consumers size:l [PR only] 101-1000 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants