-
Notifications
You must be signed in to change notification settings - Fork 415
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
Change browser support policy to Baseline Widely Available #525
Conversation
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.
Can you add something to the description to explain the change? Also, we'll have to update the "browser support" section of the README to remove mentions of IE9.
Then (either in this PR or a follow-up, though maybe this PR makes more sense?) we should make the syntax changes we want to make to modernize the code, e.g. optional chaining, for..of, nullish coalescing, etc.
Done and done.
|
@tunetheweb I made some updates, can you re-review? |
LGTM |
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.
I'm still looking, but this seems to be the only possible issue so far, so flushing the review early
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.
LGTM! Mostly a couple of questions and clarifications
src/lib/getNavigationEntry.ts
Outdated
self.performance && | ||
performance.getEntriesByType && | ||
performance.getEntriesByType('navigation')[0]; | ||
globalThis.performance?.getEntriesByType?.('navigation')[0]; |
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.
Shouldn't globalThis.performance
definitely be baseline widely available?
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.
Yeah, it's is baseline widely available, but we'd previously fixed these bugs, so that's why I was keeping it in:
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.
I wonder if it’s worth testing this branch in Opera Mini to see if there’s going to be any other similar issues?
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.
So, I just tested this in Opera Mini (with extreme data saver mode on) and discovered some interesting things.
- Looks like
performance.now()
is supported, so I guess they must have added that since I last tested. - ES6 is not supported, so if we're going to be publishing ES6 then we probably don't need to worry about other quirks of Opera Mini on extreme data saver mode
- web-vitals v3 runs without errors, but v4 doesn't work for some reason (not sure what).
When I don't turn on extreme data saver mode, all of the code runs just fine since Opera uses Chromium.
Given all of this, and our intention to support Baseline Widely Available, I think it's fine to start accessing globalThis.performance
without conditional check.
Anyone have concerns?
Change
web-vitals
builds to support Baseline Widely Available—that is features available in the major browsers 30 months ago or more. This represents between 95.93%-98.69% of desktop browsers, and between 97.32%-99.15% of mobile browsers, according to RUM Archive.Users wishing broader support can import and/or build the library themselves targeting more browsers but at the cost of larger bundle size for transpiled syntax, and with many of the features of web-vitals.js either unavailable, only partially available, or available with bugs in older browsers.