-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Multiple statements with flags #16598
Comments
Actually, looking at what the linter does, the origin of this issue isn't really about preview at all, as the following is equally accepted, contrary to #16287:
|
#17005 added a lint for this but decided to ignore flags, so your above statements still pass in BCD today. We can debate if that's a good choice or not. If you remove
your above examples will fail. In existing data, you'll only get very few failures in the CI:
So, we could investigate if these 6 are non-sense or if they actually make sense. |
There are now many more such cases:
And it looks like those
Edit: The approach in |
Looking at Firefox support for
I think we might want to update the linter to forbid multiple "open" statements of the same "dimension", e.g.: // Valid
[
{
"version_added": "preview"
},
{
"version_added": "97",
"flags": [
{
"type": "preference",
"name": "dom.serviceWorkers.navigationPreload.enabled",
"value_to_set": "true"
}
]
}
]
// Invalid
[
,
{
"version_added": "100",
"flags": [
{
"type": "preference",
"name": "dom.serviceWorkers.navigationPreload.enabled",
"value_to_set": "true"
}
]
},
{
"version_added": "97",
"flags": [
{
"type": "preference",
"name": "dom.serviceWorkers.navigationPreload.enabled",
"value_to_set": "true"
}
]
}
] Effectively, this would forbid the approach used for the ML features. |
The ML statements use this to record different compat data for different operating systems or chips (NPU, GPU, CPU) as BCD doesn't offer any other way. |
Checked all non-ML features:
|
For {
"version_added": "116",
"flags": [
{
"name": "#web-machine-learning-neural-network",
"type": "preference",
"value_to_set": "Enabled"
}
],
"notes": [
"Supports GPUs and NPUs since Chrome 121 (Windows) and Chrome 126 (macOS)",
"Supports CPUs since Chrome 116 (Windows, ChromeOS and Linux) and Chrome 126 (macOS)."
]
} ... or by OS ... {
"version_added": "116",
"flags": [
{
"name": "#web-machine-learning-neural-network",
"type": "preference",
"value_to_set": "Enabled"
}
],
"notes": [
"Supports Windows since Chrome 116 (CPUs) and Chrome 121 (GPUs, NPUs).",
"Supports macOS since Chrome 126 (CPUs, GPUs, NPUs).",
"Supports ChromeOS since Chrome 116 (only CPUs).",
"Supports Linux since Chrome 116 (only CPUs).",
]
} I would argue that this is more readable/comprehensible than having multiple statements. |
Single statements with multiple notes would work for me, too. I forgot if https://webmachinelearning.github.io/webnn-status/ was generated by BCD data or if it was the other way around and @ibelem was generating BCD data from https://webmachinelearning.github.io/webnn-status/. @ibelem do you have thoughts on the BCD data structure? Would @caugner's structure in the comment above work for you? |
I managed to accidentally have the following in a PR:
This is somewhat clearly nonsense.
Per our logic,
preview
>16
, and it doesn't make sense for it to be added in 16, and also added in a later version behind a flag without being removed from the earlier version.This should also be raising an error due to the ordering, given
preview
>16
.The text was updated successfully, but these errors were encountered: