Fix AllowBrowser versions #51121
Merged
Fix AllowBrowser versions #51121
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think this update aligns with the intent here, to cover the supported versions for those list of features mentioned. We probably want to avoid encouraging more PRs to update this list, but that feature-set seems to be the deal breaker. Initially the badge one confused me but this table makes sense, why it's not fully implemented:
|
|
I'll take the change to Chrome 120 and Opera 106. But badging just being a spec imo doesn't matter. It's a feature that's available on those modern browsers. We don't need 100% support for that to be useful. Happy to see someone explore a better useragent detector, but that's not a blocker for this. |
css-nesting is not fully supported until Chrome 120, Opera 106: https://caniuse.com/css-nesting https://developer.mozilla.org/en-US/docs/Web/CSS/Nesting_selector#browser_compatibility
ff02912 to
dc34e29
Compare
rafaelfranca
added a commit
that referenced
this pull request
May 24, 2024
…sions Fix AllowBrowser versions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Background
The new AllowBrowser feature was added in #50505 . It permits setting minimum browser versions for an application, defines and documents a
:modernversion-set as requiring a list of certain features, and adds that option to the default config for new apps.This PR has been created to make two minor corrections to
:modern, one to its versions, one to its documentation.Detail
:modernbrowser versions to match the description of required features. Specifically, CSS nesting isn't supported by Chrome until 120, Opera until 106 (1, 2).Additional information
Personally I'm not convinced this feature will be a good default for new apps, but if it's going to be offered, it should be correct for its stated purpose.
For fun, I benchmarked
UserAgent.parse.browseron my laptop and it runs in about 40 microseconds, which is faster than I'd expected. But the runtimerequire "useragent"added 30 milliseconds to the first request.The
useragentgem was last updated five years ago, and its browser recognition code is all 7 to 9 years old. It seems inappropriate to have a:moderndefault that doesn't recognize Edge (2015). There's a fork by art19 that recognizes Edge, but it adds 54 other browsers too, so it has 4x the LoC, and itsrequireis 90 milliseconds. Something in-between might be nice.I made a google spreadsheet listing browser versions that support each of the
:modernfeatures listed. The tl;dr is that badges and css nesting are the most-restrictive requirement for every major browser except Firefox (which was late to support css :has).