JakeChampion/polyfill-library

ResizeObserver polyfill does not work when bundled

wesgro opened this issue · 10 comments

wesgro commented

What

ResizeObserver is undefined when the RO polyfill script is used.

repro: https://codesandbox.io/s/polyfill-io-resizeobserver-failing-to-install-t3pwln?file=/src/polyfill.js

Details

Required setup:

  • Use Safari (or any browser that doesn't have Resize Observer)
  • Go to Safari devtools and disable Resize Observer
    ** Develop > Experimental Features > Uncheck Resize Observer
  • go to https://t3pwln.csb.app/

Upon visiting that page you'll see ReferenceError: Can't find variable: ResizeObserver

I'm at a bit of a loss why this isn't working.

What I've done is just copy/paste the output from https://polyfill.io/v3/polyfill.js?version=3.110.1&features=ResizeObserver using a spoofed UA of

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0.2 Safari/E7FBAF

But whats really mind bending is if I use the polyfill.io script in the index.html the polyfill works as expected.

What am I missing here?

romainmenke commented

Hi @wesgro,

I am unsure what the issue is and I can not run your reproduction because the code sandbox doesn't work in older browsers.

If I check with a minimal repro in an older browser everything seems to work.

<!DOCTYPE html>
<html lang="en">
<head>
	<meta charset="UTF-8">
	<meta http-equiv="X-UA-Compatible" content="IE=edge">
	<meta name="viewport" content="width=device-width, initial-scale=1.0">
	<title>Document</title>

	<script type="text/javascript" src="https://polyfill.io/v3/polyfill.min.js?version=3.110.1&features=ResizeObserver"></script>
</head>
<body>
	<script>
		console.log(ResizeObserver);
	</script>
</body>
</html>

In Chrome 62: f ResizeObserver () { [polyfill code] }
In Safari 12: function ResizeObserver () { [polyfill code] }

  • Use Safari (or any browser that doesn't have Resize Observer)
  • Go to Safari devtools and disable Resize Observer
    ** Develop > Experimental Features > Uncheck Resize Observer

When you do this you are likely still using a modern Safari that doesn't require ResizeObserver.
Is the UA altered when testing like this or not?

wesgro commented

Apologies, this appears to be something with how we bundle the polyfills that breaks the UMD in this case. Will investigate locally.

  • 🎉1
romainmenke commented

That is fine!
If it keeps giving you trouble, please let us know.

wesgro commented

It does seem to be consistently breaking whenever it gets bundled by webpack. I may open a PR to adjust how it acquires a references to the ResizeObserver polyfill (using exports instead of hoping ResizeObserver is now on the global scope)

wesgro commented

Currently the only way I have to repro is using a modern version of Safari and turning off ResizeObserver in its experimental feature settings.

But if you follow along you'll see the same error here:

The polyfill.js file is a copy/paste of exactly what polyfill.io gives me.

However when it gets bundled this breaks the reference to ResizeObserver which is patched into the library code from https://github.com/Financial-Times/polyfill-library/blob/master/polyfills/ResizeObserver/update.task.js

So it's not a problem if you use code from the polyfill.io service, it becomes one when you use this library locally and bundle its output with other modules.

romainmenke commented

I am afraid this might be a parcel issue.
They mangle the polyfill quite badly.

One of the things they seem to do is try to encapsulate it as a module and try to prevent it having global effects. This is exactly the opposite of what you want, the polyfill should be global.

I also tried this with webpack and that bundles just fine.

I have an idea for something that might help :

	exports.ResizeObserver = ResizeObserver;
	exports.ResizeObserverEntry = ResizeObserverEntry;
	exports.ResizeObserverSize = ResizeObserverSize;

+	global.ResizeObserver = ResizeObserver;
+	global.ResizeObserverEntry = ResizeObserverEntry;
+	global.ResizeObserverSize = ResizeObserverSize;

But this does not take away all the mangling that Parcel does and we also don't have a way to verify that your use case works and keeps working in the future.

Ideally Parcel exposes a way to leave polyfill files as-is.
@devongovett is this possible?

romainmenke commented

@devongovett I think you can ignore this, found a way to make it work without any changes.

@wesgro what I did :

  • use latest parcel (not v1)
  • use <script src="polyfill.js"></script>

Do not use <script type="module" src="polyfill.js"></script>
After that the polyfill just works.

There might be other combo's that work, but I think this illustrates that this is caused by incorrect configuration/usage and not something wrong with the polyfill persé.

wesgro commented

Unfortunately I don't think this a parcel only problem. I have another test case using create react app (which afaik uses webpack 5): https://github.com/wesgro/resize-observer-failing-in-cra

Using the same Safari setup you'll see that ReferenceError: Can't find variable: ResizeObserver error appearing again.

The fix propsed in #1239 (comment) would be great to get in.
I also have a proposed PR in https://github.com/wesgro/polyfill-library/tree/pr/1239 that takes a slightly different route.

While this could be fixed with different bundler configs I don't think it's reasonable to expect users to know about this potential footgun

romainmenke commented

While this could be fixed with different bundler configs I don't think it's reasonable to expect users to know about this potential footgun

I agree :)

I firstly wanted to check if this was an absolute problem (polyfill is always broken when bundled). But it seems that it sometimes works and sometimes doesn't.

I might be mistaken about this but afaik the output of this library is intended to be used as-is. Whether it works or not when bundled is unknown because we don't test for that.
We only test that the direct output works as intended.

All this combined means that it's tricky to fix.

Ideally we have some way to test and verify polyfills when bundled. This guards users against regressions. Having tests of this nature is very difficult.
We also have to be careful that the fix for your setup isn't a bug for someone else.

wesgro commented

Ideally we have some way to test and verify polyfills when bundled. This guards users against regressions. Having tests of this nature is very difficult.
We also have to be careful that the fix for your setup isn't a bug for someone else.

Agree! This is a tricky problem..

On my end I can always patch the package to modify the RO implementation to work for us if a change here is seen as too risky. So far changing it to use exports has been fruitful but I can't guarantee anything outside of my usage