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

Group BCD keys in dist by status block #1231

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

foolip
Copy link
Collaborator

@foolip foolip commented Jun 11, 2024

No description provided.

@github-actions github-actions bot added the feature definition Creating or defining new features or groups of features. label Jun 11, 2024
@foolip
Copy link
Collaborator Author

foolip commented Jun 11, 2024

This depends on #1204.

features/trusted-types.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

This is really cool!

features/abortable-fetch.yml.dist Outdated Show resolved Hide resolved
@@ -12,3 +12,6 @@ status:
firefox_android: "124"
safari: "17.4"
safari_ios: "17.4"
compat_features:
# Same status as overall feature
Copy link
Collaborator

Choose a reason for hiding this comment

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

A nice-to-have thing would be to omit this when there's only one group of keys (e.g., every single single-entry feature, but also features where all of the keys landed at the same time).

scripts/dist.ts Outdated Show resolved Hide resolved
@foolip foolip force-pushed the group-bcd-with-comments branch 2 times, most recently from 5e76648 to d7ed301 Compare June 12, 2024 22:52
@foolip foolip mentioned this pull request Jun 13, 2024
5 tasks
@foolip foolip force-pushed the group-bcd-with-comments branch 2 times, most recently from 054707e to 5cbbb1b Compare June 27, 2024 09:41
@@ -14,4 +14,15 @@ status:
safari: ≤5
safari_ios: ≤4.2
compat_features:
# baseline: high
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 notably does not say "Same status as overall feature" because the use_ranges_before_baseline_low_date logic only runs for the overall feature. @ddbeck I see a few options:

  • Add a useRangesBeforeBaselineLowDate parameter to computeBaseline. This would fix the problem locally, but users of computeBaseline don't know what to pass in. You shouldn't really be able to pick if you want the ranges or not as a consumer, that's something we do as providers of the data.
  • Revert Allow use of ranges (≤) before the Baseline low date #1208 to not block progress on this more important PR and worry about it later.

WDYT?

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've sent #1286.

Copy link
Collaborator

@ddbeck ddbeck Jun 27, 2024

Choose a reason for hiding this comment

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

I agree on the revert and I just approved it. I need to think on the broader problem. I think doing #1211 — which would just make ranges a dumb fact of the status calculation — is the right way to go, but that's more of a guess right now.

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 agree, #1211 seems like the better first step. Then we can also allow ranges in support overrides, and see how we end up using it.

@foolip
Copy link
Collaborator Author

foolip commented Jun 27, 2024

The main open question is the sort order of blocks.

We should sort first by Baseline date, oldest first. But among blocks with the same Baseline date, what's the secondary sort key?

One approach is to use all of the (between 0 and 7) releases dates to sort. The most recent date is the Baseline low date, and if that's the same we move on to compare the 2nd most recent date, and so on. The issue is that two browsers can have the same release date. For example, Chrome 126 and Firefox 127 were both released on 2024-06-11, and this seems to be common now. Using dates only won't produce unique sort keys in every case where browser versions differ.

Any way of breaking that tie should be fine, but I don't have a concrete way in mind right now...

@foolip foolip force-pushed the group-bcd-with-comments branch 2 times, most recently from 9e7e05a to bebcb5d Compare June 27, 2024 19:14
@ddbeck
Copy link
Collaborator

ddbeck commented Jun 28, 2024

Huh, this is complicated. I think I'd like to see the overall sort look like this:

  1. Baseline low date blocks, oldest first.
  2. After all Baseline blocks, sorted by number of engines.

Break ties within 1 and 2 by:

a. Number of compat keys, greater first.
b. Next last-implementer release date, older first.
c. Lexical ordering of the first compat key in each block.

Taken together, this ought to the most-widely available things first, with bigger slices before smaller ones. Then it's back to date ordered, with something very arbitrary as a fallback.

@foolip foolip force-pushed the group-bcd-with-comments branch 3 times, most recently from 814f60c to 2fc1bac Compare June 29, 2024 19:05
@foolip
Copy link
Collaborator Author

foolip commented Jun 29, 2024

@ddbeck I've implemented a sort order now, it's the same as you suggested except the tie breaking. I wanted to avoid this in particular:

Next last-implementer release date, older first.

This information isn't visible in the dist files, and I think it might be hard to understand the order if it uses inferred information. Adding dates in comments would address this, if you think this is important.

Instead I just sorted by the first non-equal version when the number of supporting browsers is the same. This is done even if it's not the same browsers in two groups.

The resulting order feels alright but could probably be improved with experience reviewing with groups.

@foolip foolip marked this pull request as ready for review June 29, 2024 19:12
ddbeck added a commit to ddbeck/web-features that referenced this pull request Jul 1, 2024
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

@foolip I'm really appreciating the new sort. I reviewed the dist files from A to C. I think this is really close, but I noticed a couple of oddities.

features/aspect-ratio.yml.dist Outdated Show resolved Hide resolved
Comment on lines +17 to +40
# baseline: high
# baseline_low_date: 2015-07-29
# baseline_high_date: 2018-01-29
# support:
# chrome: "15"
# chrome_android: "18"
# edge: "12"
# firefox: "13"
# firefox_android: "14"
# safari: "6"
# safari_ios: "6"
- css.properties.border-image-width

# baseline: high
# baseline_low_date: 2015-07-29
# baseline_high_date: 2018-01-29
# support:
# chrome: "15"
# chrome_android: "18"
# edge: "12"
# firefox: "15"
# firefox_android: "15"
# safari: "6"
# safari_ios: "6"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah this is cool! This is very likely a bug in the data, right? It seems deeply unlikely that Firefox shipped border-image-width before border-image-source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. This grouping will throw up a lot of issues like this. If we try to fix them I'm sure we'll move slower instead of faster. Do we need some fast way to mark issues like this as future TODOs?

features/broadcast-channel.yml.dist Show resolved Hide resolved
Comment on lines +16 to +36
compat_features:
# baseline: false
# support:
# chrome: "69"
# chrome_android: "69"
# firefox: "83"
# firefox_android: "83"
# safari: "12.1"
# safari_ios: "12.2"
- css.types.image.gradient.conic-gradient
- css.types.image.gradient.repeating-conic-gradient

# baseline: false
# support:
# chrome: "72"
# chrome_android: "72"
# firefox: "83"
# firefox_android: "83"
# safari: "12.1"
# safari_ios: "12.2"
- css.types.image.gradient.conic-gradient.doubleposition
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something weird has happened here. How did we get to baseline: high without Edge?

Copy link
Collaborator

@ddbeck ddbeck Jul 1, 2024

Choose a reason for hiding this comment

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

OK, I figured this one out. You're (correctly) using getStatus for the slices, but we use computeBaseline() without checkAncestors: true on the overall feature. That means the overall status never checks css.types.image.gradient's support. And that surfaces an issue in BCD: it says that <gradient> (the abstract type!) has an -ms- prefix, which makes zero sense. So this is not an issue with this PR, but does need a fix upstream to BCD. I'll tend to that now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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've sent #1294, we shouldn't use both checkAncestors: true and checkAncestors: false in the dist script.

Comment on lines 16 to 50
# baseline: low
# baseline_low_date: 2022-03-14
# support:
# chrome: "52"
# chrome_android: "52"
# edge: "79"
# firefox: "69"
# firefox_android: "79"
# safari: "15.4"
# safari_ios: "15.4"
- css.properties.contain

# baseline: low
# baseline_low_date: 2022-07-26
# support:
# chrome: "52"
# chrome_android: "52"
# edge: "79"
# firefox: "103"
# firefox_android: "103"
# safari: "15.4"
# safari_ios: "15.4"
- css.properties.contain.style

# baseline: low
# baseline_low_date: 2022-09-12
# support:
# chrome: "105"
# chrome_android: "105"
# edge: "105"
# firefox: "101"
# firefox_android: "101"
# safari: "16"
# safari_ios: "16"
- css.properties.contain.inline-size
Copy link
Collaborator

Choose a reason for hiding this comment

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

These keys have no business being there and these comments make it obvious. Nice! Sent #1290

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

OK, now that I've figured out the story behind the phantom gradient in Edge, I'm marking this as approved. But if you want to do anything about #1231 (comment), I would welcome that.

foolip pushed a commit that referenced this pull request Jul 1, 2024
@foolip foolip merged commit 615827c into web-platform-dx:main Jul 1, 2024
3 checks passed
@foolip foolip deleted the group-bcd-with-comments branch July 1, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature definition Creating or defining new features or groups of features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants