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.

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

fix: do not use URLSearchParams to support IE 11 without polyfill #106

Merged
merged 4 commits into from
Jun 20, 2020
Merged

fix: do not use URLSearchParams to support IE 11 without polyfill #106

merged 4 commits into from
Jun 20, 2020

Conversation

iiroj
Copy link
Contributor

@iiroj iiroj commented Jun 5, 2020

As mentioned in #88 (comment), we kept having issues with the top-level URLSearchParams call in ErrorOverlayEntry.js. Because it gets immediately invoked when the webpack chunk containing the code is loaded, it was running before our polyfill bundle and thus breaking IE 11.

Since the purpose of the offending code was to parse override options from the webpack __resourceQuery, I opened this PR to avoid the call entirely and instead just using decoreURIComponent, and String.prototype.split.

This should allow the plugin to work without polyfills on IE 11.

What do you think?

I'm not sure if the README has to be updated with regards to the polyfilll section.

@pmmmwh
Copy link
Owner

pmmmwh commented Jun 7, 2020

Some background:

  • __resourceQuery parsing is exclusively used for webpack-dev-server only
  • Currently, the socket implementation for webpack-dev-server will depend on URL (inferred from usage of native-url)

The two above combined will mean even with this change, IE is still imcompatible, unless we do a custom implementation of url.format. I can add a shim to this myself later.

All that said - I think we can move the __resourceQuery parsing to WDSSocket.js.

@iiroj
Copy link
Contributor Author

iiroj commented Jun 7, 2020

Thanks for the comment. I looked at the usage of native-url, since we do use webpack-dev-server and the websocket implementation (instead of the default sockjs). It seems that part of the code then works fine on IE 11, at least in our case.

All in all, with this change it solved our problems.

I'm fine with it being solved another way though!

@iiroj iiroj marked this pull request as draft June 7, 2020 12:48
@iiroj
Copy link
Contributor Author

iiroj commented Jun 7, 2020

@pmmmwh I converted this PR into a draft to indicate it's not ready to be merged.

If I understood you correctly, I pushed a few further refactoring commits to move the parsing into a util method that's called internally in WDSSocket.js.

If you rather wish to take care of this yourself, let me know!

@pmmmwh
Copy link
Owner

pmmmwh commented Jun 8, 2020

If I understood you correctly, I pushed a few further refactoring commits to move the parsing into a util method that's called internally in WDSSocket.js.

Yea, this is how I've envisioned it. I'll think about chunking order issues.

I think moving forward maybe we'll have to have logic to make sure the error overlay is injected once only, like how the refresh hook should only be injected once. This can be handled later.

@pmmmwh pmmmwh changed the base branch from master to main June 11, 2020 10:38
@pmmmwh pmmmwh closed this Jun 11, 2020
@pmmmwh pmmmwh reopened this Jun 11, 2020
@iiroj iiroj marked this pull request as ready for review June 12, 2020 06:22
@iiroj
Copy link
Contributor Author

iiroj commented Jun 12, 2020

Hello,

I rebased this PR to the new main branch, and removed the draft status, since I had some time to test that everything is still working in IE 11 (and others).

@iiroj
Copy link
Contributor Author

iiroj commented Jun 12, 2020

I also opted to refactor the parseURLSearchParams into getResourceQuery that instead of being generic, only returns the parsed __resourceQuery global variable. I also added some tests for it.

@pmmmwh pmmmwh merged commit d698bbf into pmmmwh:main Jun 20, 2020
@pmmmwh
Copy link
Owner

pmmmwh commented Jun 20, 2020

Thanks for the PR! This will land in the next beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants