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

Adds ability to check for possible updates across multiple default statements #967

Merged
merged 12 commits into from
Dec 21, 2023

Conversation

ChrisC
Copy link
Contributor

@ChrisC ChrisC commented Dec 14, 2023

This continues from the work in #909 and implements a request by @foolip to check for possible updates across multiple default statements in the new preliminary hasSupportUpdates check.

These new test cases show what happens with multiple default statements with different combinations of test results.

The update script now logs more warnings from cases where a possible update was detected in more complex scenarios, but was not completed by the update script.

cc @foolip

scripts/update-bcd.ts Outdated Show resolved Hide resolved
scripts/update-bcd.ts Outdated Show resolved Hide resolved
scripts/update-bcd.ts Show resolved Hide resolved
@foolip
Copy link
Member

foolip commented Dec 20, 2023

@ChrisC let me try to summarize succinctly what it is I think we should do here:

  • Define what it means for a single test result (true/false/null for a specific browser version) to "have support updates for" a BCD supports statement. We can treat that support statement as an array for simplicity, even if it's an array of one element. Silences warning logs and adds additional earlier skip check if no detected BCD updates  #909 (comment) is a sketch of this that doesn't handle version_added being "preview", true, or "≤" ranges.
  • Run that test against defaultStatements before we do anything, and do nothing if there are no updates needed.
  • Run all of the updating machinery, which infers support statements, etc.
  • Before writing the updated JSON to disk, filter it in the same way as defaultStatements and apply the test again. Warn about cases that are still not updated which will need manual intervention.

@ChrisC
Copy link
Contributor Author

ChrisC commented Dec 20, 2023

@foolip Good news! I ran this sample code through the multiple-statement test case and verified that it does work 👍🏽

I think I missed that this loop continues iterating through all the statements until we find a confirmation of support per version, and only returns false after we've iterated through the array of statements without finding any matching support. That was a slightly different approach from what I tried earlier that I didn't see at first glance.

I'll go ahead and integrate this approach, add the handling for possible booleans/ "preview" values, and remove the support range utility method .

in favor of simpler support detection across multiple default statements
adds additional boolean & string test cases
for use in the `hasSupportUpdates` checks that run before and after `persist...` operations
@ChrisC
Copy link
Contributor Author

ChrisC commented Dec 20, 2023

@foolip I believe I got in all of the major requested changes. Feel free to take another look at this!

scripts/update-bcd.test.ts Outdated Show resolved Hide resolved
scripts/update-bcd.test.ts Outdated Show resolved Hide resolved
scripts/update-bcd.test.ts Outdated Show resolved Hide resolved
scripts/update-bcd.test.ts Outdated Show resolved Hide resolved
return false;
}

// In the case of general boolean statements, only show support if the version from the test result does not show specific support, otherwise we should ignore generic boolean support statements in favor of specific version support info from a test result
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree this makes sense. It's a bit unfortunate that hasSupport is passed in, but it makes sense here. Maybe it can be refactored somehow, but I'm not sure how right now, and this works!

}) => {
if (!statements?.length) {
if (hasSupportUpdates(versionMap, simpleStatement)) {
if (hasSupportUpdates(versionMap, unmodifiedDefaultStatements)) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, wait, going just by the name, isn't this the original BCD data (without any modifications) and not the updated BCD data that we're about to write back to disk? It needs to be the updated data, but filtered once again to "default statements", meaning excluding things like prefixed statements, flags, etc.

Copy link
Contributor Author

@ChrisC ChrisC Dec 21, 2023

Choose a reason for hiding this comment

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

Yes that's correct. Seems like I misunderstood the requirement here. We were checking the updated, filtered "default statements" before this change, so I'll walk this back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok addressed here bec9963

@foolip foolip merged commit 904be18 into openwebdocs:main Dec 21, 2023
2 checks passed
@foolip
Copy link
Member

foolip commented Dec 21, 2023

I've tested this and there are now very few warnings, only 3:

warn: api.EXT_texture_compression_rgtc skipped for firefox with unresolved differences between support matrix and BCD data. Possible intervention required.
warn: api.HTMLCanvasElement.getContext.webgl2_context.options_powerPreference_parameter skipped for firefox with unresolved differences between support matrix and BCD data. Possible intervention required.
warn: api.HTMLElement.autofocus skipped for firefox with unresolved differences between support matrix and BCD data. Possible intervention required.

I took a look at the last one, and found that it was actionable and that fixing it silenced the warning:
mdn/browser-compat-data#21707

This is very nice, thanks @ChrisC!

@ChrisC
Copy link
Contributor Author

ChrisC commented Dec 21, 2023

I took a look at the last one, and found that it was actionable and that fixing it silenced the warning: mdn/browser-compat-data#21707

That's great to hear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants