Skip to content

Conversation

@foolip
Copy link
Collaborator

@foolip foolip commented Jul 5, 2024

No description provided.

@github-actions github-actions bot added the feature definition Creating or defining new features or groups of features. label Jul 5, 2024
@foolip foolip force-pushed the more-dist branch 6 times, most recently from 08f5775 to eb8608b Compare July 5, 2024 13:58
Copy link
Collaborator Author

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Self-review to make this easier to check for others.


status:
baseline: high
baseline_low_date: 2015-09-30
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 change from the previous 2016-09-20, but I think this is the right thing to do for consistency. The other array features matched Array, not TypedArray, if both were part of the feature. See below.


status:
baseline: high
baseline_low_date: 2015-09-01
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Date and versions matches previous status override.


status:
baseline: high
baseline_low_date: 2015-09-01
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Date and versions matches previous status override.


status:
baseline: high
baseline_low_date: 2016-09-20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Date and versions matches previous status override.

baseline: high
baseline_low_date: 2016-09-20
baseline_high_date: 2019-03-20
baseline_low_date: 2015-09-30
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Existing dist file, I added compute_from to apply a consistent approach, and it changes the Baseline low date.


status:
baseline: high
baseline_low_date: 2015-07-29
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unchanged.

baseline_low_date: 2020-01-15
baseline_high_date: 2022-07-15
support:
chrome: "53"
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 changed slightly but now matches https://caniuse.com/shadowdomv1. Ditto for Safari below.

chrome: "45"
chrome_android: "45"
edge: "12"
firefox: "38"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Firefox and Safari versions changed, and Baseline low date too as a result. That seems OK in this case because filter() and friends feel like they're on equal footing with every() and friends that shipped slightly earlier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Validating to see cases where we're not reaching from compute_from. 👍


status:
baseline: high
baseline_low_date: 2016-09-20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unchanged due to compute_from: javascript.builtins.TypedArray.@@iterator, but notably the description points to entries() and including that would change the status. It would be good to handle this and array-iterators.yml in the same way, however.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The description really strongly implies we're treating those things as core to the feature. Either the description needs to change, or we need to bring entries() along for the ride. Feels more honest to do the latter, to change the status rather than reshape the feature.


status:
baseline: high
baseline_low_date: 2015-07-29
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 unchanged and matches https://caniuse.com/typedarrays except for Safari, where caniuse says that Safari 5.1 didn't support Float64Array. I haven't checked if that's right.

@foolip foolip marked this pull request as ready for review July 5, 2024 14:00
@foolip foolip requested a review from Elchi3 July 5, 2024 14:00
@foolip
Copy link
Collaborator Author

foolip commented Jul 5, 2024

@Elchi3 can you review this? There are quite a few per-feature decisions for JavaScript here, and I tried to be consistent. But consistency sometimes leads to weird places. I think it's OK, but am happy to change whatever you think isn't optimal.

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.

I'm OK with pretty much all of this, but leaving unmerged for Florian's review.

chrome: "45"
chrome_android: "45"
edge: "12"
firefox: "38"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Validating to see cases where we're not reaching from compute_from. 👍


status:
baseline: high
baseline_low_date: 2016-09-20
Copy link
Collaborator

Choose a reason for hiding this comment

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

The description really strongly implies we're treating those things as core to the feature. Either the description needs to change, or we need to bring entries() along for the ride. Feels more honest to do the latter, to change the status rather than reshape the feature.

@Elchi3
Copy link
Collaborator

Elchi3 commented Jul 9, 2024

@Elchi3 can you review this? There are quite a few per-feature decisions for JavaScript here, and I tried to be consistent. But consistency sometimes leads to weird places. I think it's OK, but am happy to change whatever you think isn't optimal.

I checked the JavaScript features and it is great to see values computed from BCD directly. LGTM to remove all the manual overrides.

I'm not sure I understand why certain JS features use compute_from. Taking into account all features feels more honest to me. Can you elaborate why is compute_from is useful here?

@foolip
Copy link
Collaborator Author

foolip commented Jul 9, 2024

I'm not sure I understand why certain JS features use compute_from. Taking into account all features feels more honest to me. Can you elaborate why is compute_from is useful here?

I wanted to apply a consistent rule and at the same time minimize changes, and using compute_from to exclude typed arrays was the rule that resulted in the fewest changes.

I've now pushed another commit dropping all use of compute_from for JavaScript features and so you can review that instead. If you prefer it, it's fine with me too.

Copy link
Collaborator

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

LGTM. I think my rule would be that not using compute_from is the default. If using it, it would be good to provide a reason.

Copy link
Collaborator Author

@foolip foolip left a comment

Choose a reason for hiding this comment

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

I've commented on which Baseline low dates have changed now that compute_from isn't used. The noteworthy changes are:

  • Array iterators changed from 2016 to 2018 because of Array values() in Firefox 60. That data came from mdn/browser-compat-data#1014 and seems reliable.
  • Typed arrays changed from 2015 to 2017. I find this change to be unreasonable because typed arrays are usable without most of the methods. I'll put compute_from back.


status:
baseline: high
baseline_low_date: 2016-09-20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was previously 2015-09-01


status:
baseline: high
baseline_low_date: 2016-09-20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was previously 2015-09-01


status:
baseline: high
baseline_low_date: 2018-05-09
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was previously 2016-09-20


status:
baseline: high
baseline_low_date: 2016-09-20
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was previously 2016-03-21


status:
baseline: high
baseline_low_date: 2017-08-08
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was previously 2015-07-29

@foolip
Copy link
Collaborator Author

foolip commented Jul 9, 2024

LGTM. I think my rule would be that not using compute_from is the default. If using it, it would be good to provide a reason.

Can you review again after the typed array change?

I think that with how we're authoring most features, compute_from will end up being used in the majority of features. This is because most features are usable when they first ship, and still get some additional bits of API over time that are part of the feature and not worth splitting into a separate feature, at least not long term.

#1091 is a good example of this unfolding. It would be good to give some guidance on this in #1330 too.

@Elchi3
Copy link
Collaborator

Elchi3 commented Jul 9, 2024

Can you review again after the typed array change?

I think this works for me. Take my comments as thoughts as I'm learning about compute_from and not as a blocker.

I think that with how we're authoring most features, compute_from will end up being used in the majority of features. This is because most features are usable when they first ship, and still get some additional bits of API over time that are part of the feature and not worth splitting into a separate feature, at least not long term.

Interesting. I guess for typed arrays the alternative would be to have two groups:

  • "Initial typed arrays" with the constructors that you've now put in compute_from.
  • "ECMAScript 2015 typed arrays" containing the additional methods etc. that typed arrays gained once they were moved from their own Khronos spec to the ECMAScript spec.

So, I guess your point is that we don't need these two groups but instead just one group with all BCD keys and then compute_from does the baseline computation based on a subset of BCD keys.

It would be good to give some guidance on this in #1330 too.

Guidelines when to opt into compute_from and not into separate features would be appreciated.

#1091 is a good example of this unfolding.

Interesting to see compute_from with a single key in that PR. I would have expected all keys except json_static to be in compute_from. (or have something like ignored_keys: api.Response.json_static).

@foolip
Copy link
Collaborator Author

foolip commented Jul 9, 2024

Interesting to see compute_from with a single key in that PR. I would have expected all keys except json_static to be in compute_from. (or have something like ignored_keys: api.Response.json_static).

@ddbeck and I have discussed this and he was initially also surprised by my minimal approach, but later came around to it. The model I have is that in order to pin a feature to its initial shape, we only need to list the entrypoints and anything additional that is necessary to use the feature at all. Anything that isn't strictly necessary is left out. The effect is the same as listing all of the things that shipped initially, but it's easier to find the right keys without reference to implementation status.

Regarding excluding keys instead, that would require a new exclusions every time a feature gains a small additional bit. To me, it seems like more work and much more likely that it will be overlooked in some case, leading to a change in the feature's Baseline low date.

@foolip foolip merged commit 8deba1e into web-platform-dx:main Jul 9, 2024
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.

3 participants