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.

Sign up for GitHub

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

Jump to bottom

polyfill usage with /base is counterintuitive #95

Closed
roippi opened this issue Nov 20, 2020 · 6 comments
Closed

polyfill usage with /base is counterintuitive #95

roippi opened this issue Nov 20, 2020 · 6 comments

Comments

@roippi
Copy link

roippi commented Nov 20, 2020

Hi there. The how the polyfill works section states:

The "standard" version of the web-vitals library includes some of the same logic found in polyfill.js

This means that you must include the polyfill, even on browsers (ie chrome) that natively support First Input Delay. Omitting the polyfill will break FID on chrome. (Cannot read property 'firstHiddenTime' of undefined)

This is rather counterintuitive: it means the polyfill is doing more than just polyfilling. It's bundling some application logic in with the polyfill. I've never seen another polyfill on the web that acts this way - a reasonable developer expectation is that polyfills replace missing browser functionality, and no more.

Can we make the polyfill pure, and leave application logic inside the app? Alternately, calling this something other than a "polyfill" since it's really a polyfill + helper script. Thanks.
@philipwalton
Copy link
Member

This means that you must include the polyfill, even on browsers (ie chrome) that natively support First Input Delay. Omitting the polyfill will break FID on chrome. (Cannot read property 'firstHiddenTime' of undefined)

You do not need to include the polyfill script or use web-vitals/base. If you do not include the polyfill, FID will work just fine in Chromium-based browsers.

The only way it will break is if you do not add the polyfill script but then you import web-vitals/base in your code. The reason this will break is because the "base" version assumes the polyfill script has already been loaded on the page. (I'm assuming that's how you're seeing the Cannot read property 'firstHiddenTime' of undefined error?)

I thought it was clear in the documentation that it had to be one or the other. If you think it's not clear, let me know.

This is rather counterintuitive: it means the polyfill is doing more than just polyfilling. It's bundling some application logic in with the polyfill.

The polyfill is just polyfilling. Where I agree it's confusing is that the "standard" version is also polyfilling—it's just not polyfilling as much stuff. What it's polyfilling is the detection of the metrics after a bfcache restore.

The only reason there are two scripts is because regular FID cannot be polyfilled without adding code to the <head> of the document, whereas bfcache-restore FID can be polyfilled as long as the script is loaded prior to the page being put in the cache (which it always will be).

And since some of the code required to detect FID cross-browser is also required to detect FID after a bfcache restore, it made sense to have two versions of the script to avoid duplication when using the polyfill. I agree it'd be simpler to have just one version, but I thought it was worth a bit of confusion to avoid the duplication.

Once there are APIs to detect bfcache versions of these metrics, then the web-vitals/base version can go away and it will then be a standard script plus an optional polyfill to support other browsers. We're kinda in a weird state right now, unfortunately.
@roippi
Copy link
Author

roippi commented Nov 21, 2020 •
edited
Loading

You do not need to include the polyfill script or use web-vitals/base.

I work for a large e-commerce website; we want to capture webvitals for all of our customers. 60-70% of our mobile traffic is safari.

It is not feasible to bundle different versions of webvitals for different browsers.

Thus, we need to use web-vitals/base. This breaks in chrome if we do not include the polyfill.

I thought it was clear in the documentation that it had to be one or the other. If you think it's not clear, let me know.

It is clear. It is also counterintuitive, and not everyone reads every letter of the documentation when implementing. We didn't. It caused our webvitals to break for a bit, because we omitted the polyfill for chrome.

I do not think it is unreasonable to ask for something labeled a polyfill to be a pure polyfill.

The polyfill is just polyfilling

web-vitals/base does not work without the polyfill on chrome. FID breaks with the error Cannot read property 'firstHiddenTime' of undefined.
@philipwalton
Copy link
Member

I work for a large e-commerce website; we want to capture webvitals for all of our customers. 60-70% of our mobile traffic is safari.

It is not feasible to bundle different versions of webvitals for different browsers.

Thus, we need to use web-vitals/base. This breaks in chrome if we do not include the polyfill.

Yes, that's correct. This is expected, and this is what is documented. Shipping web-vitals/base to all users and the polyfill to just Chrome users is not supported.

Also, keep in mind that even Chrome benefits from the polyfill, as it gets a (potentially) more accurate recording of the page's visibility state when it was loaded. In addition, it could be the case in the future that the polyfill is ahead of Chrome, so it's not a safe assumption that Chrome will never need the polyfill.

If you want to use the polyfill and get Safari support, then you have to use it with the web-vitals/base script. If you want to ship different versions to different browsers, then you have to do that in both places (your JS bundle and the HTML document).

It is clear. It is also counterintuitive, and not everyone reads every letter of the documentation when implementing. We didn't. It caused our webvitals to break for a bit, because we omitted the polyfill for chrome.

I do not think it is unreasonable to ask for something labeled a polyfill to be a pure polyfill.

The polyfill is a pure polyfill. Your issue was caused by using web-vitals/base in a way that's not supported.

As I said before, I agree and understand that this is not the way that libraries are typically added to a page, but there's no other way to measure FID in all browsers other than to have a separate script added to the <head> of the page.

If you have a suggestion for a better for web-vitals/base in order to make it more clear, I'd love to hear it. I originally called it web-vitals/external-polyfill (to emphasize that it was meant to be used with the external polyfill), but I got feedback from a few people on my team that it was weird to call a script "external-polyfill" if it wasn't an external polyfill.
@roippi
Copy link
Author

roippi commented Nov 23, 2020

Thanks for the response. I will be on hiatus for the next week and can reply next Monday. Hope you and everyone else reading this has a happy and safe holiday.
@noahnu
Copy link

noahnu commented Jan 26, 2021

I thought it was clear in the documentation that it had to be one or the other. If you think it's not clear, let me know.

I think it can be clarified. We ran into the same issue where we operated under the faulty assumption that in the worst case, a missing polyfill would just mean no data. I assumed I would still be able to call getFID, and that the onReport callback would just never be triggered if it wasn't supported. Instead, without the polyfill, getFID raises an uncaught exception.

It'd be valuable to update the documentation to call out that without the polyfill, use of the web vitals base script may produce undefined behaviour, and not just "missing data". Alternatively, in my opinion it's more intuitive to have the various getMetric functions just become no-ops if there is not sufficient information to trigger the onReport handler.
@philipwalton philipwalton mentioned this issue Jul 10, 2022
Closed
@tunetheweb
Copy link
Member

Closing this a the polyfill is deprecated and will be removed in a future version. Plus it's primary use case is for FID which is less relevant now that INP is due to replace it.
@tunetheweb tunetheweb closed this as not planned Won't fix, can't repro, duplicate, stale Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants
@philipwalton @noahnu @roippi @tunetheweb