Switches URL polyfill to whatwg-url

[mhassan1]

This PR replaces the URL polyfill with whatwg-url. It uses browserify to bundle and transpile whatwg-url.

Resolves #53.

[mhassan1]

@romainmenke This is ready for review.

[romainmenke]

Some notes and questions, overal looks good!

The polyfill is indeed quite large, but I think that going for correctness is the right choice here.

[mhassan1]

@romainmenke I've changed the way update.task.js works and force-pushed. Please take a look.

[mhassan1]

This PR Is blocked by #64.

[romainmenke]

LGTM!

Thank you @mhassan1 for finding an alternative here 🙇

[romainmenke]

🤔 CI seems to be more flaky now in very old browser versions.
I suspect that the size is causing issues. (network or memory?)

Is there anything you can think of we can improve here?

[mhassan1]

🤔 CI seems to be more flaky now in very old browser versions. I suspect that the size is causing issues. (network or memory?)

Is there anything you can think of we can improve here?

Flaky on URL tests? I see timeouts are being hit here. Should be just increase the timeouts for the URL tests?

[mhassan1]

For reference, here are the largest polyfills we have:

840K	polyfills/Intl/DateTimeFormat/~timeZone/all/polyfill.js
532K	polyfills/Intl/DateTimeFormat/~timeZone/golden/polyfill.js
344K	polyfills/URL/polyfill.js
148K	polyfills/Intl/Locale/polyfill.js
144K	polyfills/String/prototype/normalize/polyfill.js

So URL is not the biggest one we have. Maybe there's something else about it that makes it slow to execute.

[romainmenke]

I think that maybe it is a really really slow parser.

Probably a combination of using many other features (e.g. TextEncoder, Symbol, iterators, Set, ...) and not having even the most basic optimizations (e.g. cp2 === p(":") vs. cp2 === 58).

I am concerned that url parsing is something that can happen fairly often and frequently in some client side applications. If parsing a single URL can take anywhere between ±100ms and over 2 seconds then I am sure we will be breaking some things downstream.

What options do we have here?

[mhassan1]

I think that maybe it is a really really slow parser.

I ran the URL tests locally with all detect.js files as false (to force all polyfills to load in my modern browser). The test suite ran in 0.08s. Of course, my laptop is much more powerful than the BrowserStack machines; maybe this polyfill performs fine on moderately powerful machines running old browser versions.

I just ran that same test (all detect.js files as false and all config.toml as *) against a bunch of old Chrome versions on Windows 11. It seems like the really old Chrome versions (less than 46) are 10x slower than the others:

Windows 11
----------
Chrome 50: 1.40s
Chrome 49: 1.28s
Chrome 48: 1.84s
Chrome 47: 1.67s
Chrome 46: 1.73s
Chrome 45: 11.48s
Chrome 44: 11.54s
Chrome 43: 11.91s
Chrome 42: 11.96s
Chrome 41: 13.36s
Chrome 40: 16.67s

Windows XP
----------
Chrome 46: 1.23s
Chrome 45: 11.39s

I've narrowed down the issue to Object.defineProperty with get/set, which is much slower in Chrome 45 and below. Compare for (var i = 0; i < 5000; i++) Object.defineProperty({},0,{get:function(){return 0}}) between Chrome 45 (~4s) and 46 (instant). So, I don't think the URL parser itself is slow, but it appears slow because it allocates many ArrayBuffer instances, which call Object.defineProperty many times (link).

Chrome 45 doesn't get the ArrayBuffer polyfill, which is why it's fast when running the URL tests normally (without forcing it to get the polyfill), but Chrome 44 and below need the ArrayBuffer polyfill.

Options:

1. Increase the timeout on the URL tests for Chrome 44 and below (because they get the ArrayBuffer polyfill)
2. Drop support for Chrome 44 and below (0.03% usage total, according to https://caniuse.com/usage-table)
3. Refactor the ArrayBuffer polyfill to not use Object.defineProperty in a loop

As a test of option 3, I replaced repeated Object.defineProperty calls in the ArrayBuffer loop with Object.defineProperties, but it made no improvement.

[romainmenke]

Thank you for investigating further @mhassan1 🙇
And sorry for the delay in picking this thread back up.

At this time I think we might need to revert the changes to URL.
I don't think we can drop support for the affected browser versions and I prefer dropping support for specific versions from a position we can revert from without it being a breaking change. (having to also remove shipped features like URL.canParse would also be breaking)

I think that the value of a working URL polyfill for these very old browser versions is higher than the value of URL.parse and URL.canParse in any browser version.

Do you see a path forwards that allows us to have both this polyfill for URL and preserve support for these old browser versions?

[romainmenke]

Upon closer inspection I am more convinced that we should revert this change.
The URL polyfill is currently only used for also some very old browser versions and the number of versions that could be served the updated whatwg polyfill and still actually need it is even smaller still.

Only:

• Chrome 45-60
• Safari 10-11
• ...

I think it is fine to have a URL.parse and URL.canParse polyfill that only work fully as expected in browsers with a native URL implementation that throws in all expected cases.

[romainmenke]

I've opened a PR to revert this change here: #72

But waiting to hear your thoughts and concerns first :)
I really appreciate the time and effort you've put into this polyfill and maybe you see path forwards that I am currently overlooking.

[mhassan1]

Just to clarify: the old URL polyfill had incorrect behavior, regardless of URL.parse, so I don't think it would be better to go back to it. I think correct behavior is more important than performance.

The performance issue is caused by the frequent use of the ArrayBuffer polyfill, which seems to be slow. Maybe we could make it faster.

Or, we could have two URL polyfills that load dynamically based on the browser, but that would still mean that we are serving an incorrect polyfill to old browsers.

[mhassan1]

I've put up a PR for another proposal: remove the need for the ArrayBuffer polyfill for some browsers by removing the need for TypedArray.prototype.slice and TypedArray.prototype.indexOf: #73.