-
Notifications
You must be signed in to change notification settings - Fork 199
update-drafts uses parent spec if one is present
#1994
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
update-drafts uses parent spec if one is present
#1994
Conversation
…pdate-drafts-parent-spec
ddbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions on the code itself, but this seems like a good idea.
scripts/update-drafts.ts
Outdated
| let parentFeature = compat.data as Identifier; | ||
| while (path.length > 1) { | ||
| parentFeature = parentFeature[path.shift()]; | ||
| const parent_spec = parentFeature.__compat?.spec_url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, you can use compat.features to get an object with a spec_url getter (probably):
| let parentFeature = compat.data as Identifier; | |
| while (path.length > 1) { | |
| parentFeature = parentFeature[path.shift()]; | |
| const parent_spec = parentFeature.__compat?.spec_url; | |
| while (path.length > 1) { | |
| const parent_spec = compat.features.get(path.shift())?.spec_url ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't quite work- it was calling compat.features.get('properties') instead of compat.features.get('css.properties'). I did switch to building a path up and using compat.features.get() instead of drilling through the raw data.
ddbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'm happy with this, but I have a question about webgpu.
Also, I merged the update-drafts workflow earlier today—you might need to merge main into this branch and rerun the script to avoid conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were the changes to webgpu done by hand or caused by the script? This is the only thing that worries me—I want to preserve the mostly hands-off use of the update-drafts Actions workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question on webgpu. #1948 moved the draft to the features folder, and because this PR was updating the draft, merging that move in added the keys to the non-draft feature.
This PR doesn't increase the likelihood that that would happen in the future, but this PR is where it's most likely to happen. It's slightly mitigated by the fact that the action only adds the draft folder, but a simultaneous move could theoretically add keys to a non-draft feature.
I reset features/webgpu.yml to match main, and updated the drafts, so now the new keys are in features/draft/spec/webgpu.yml.
…pdate-drafts-parent-spec
ddbeck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let's go!
Before, if a BCD key didn't have a spec_url, it did not show up in the draft features. With this change, if a BCD key doesn't have a spec_url, it will use the closest ancestor's spec.
Apologies for the diff and for resurrecting deleted drafts, but a quick glance looks like most will be simple additions to existing specs.
These don't have a spec or an ancestor with a spec
Notes: for
-webkit-mask-box-image-*, these will be removed pending mdn/browser-compat-data#22727.