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

Add EventSource polyfill to hot-middleware-client #3945

Merged
merged 11 commits into from
Mar 6, 2018

Conversation

jstcki
Copy link
Contributor

@jstcki jstcki commented Mar 6, 2018

@timneutkens
Copy link
Member

Can be done with https://github.com/zeit/next.js/tree/canary/examples/with-polyfills

Not sure if we want to do this by default 🤔cc @arunoda

@jstcki
Copy link
Contributor Author

jstcki commented Mar 6, 2018

Since this doesn't affect app code IMHO it should be polyfilled by the framework. Also, it's only included in dev mode, so no overhead is added to production.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides my earlier comment, code looks great.

@arunoda
Copy link
Contributor

arunoda commented Mar 6, 2018

@herrstucki we haven't tested dev mode a lot with IE11 assuming we use it for production and use Chrome/FF for dev.

Anyway, this looks legit to me.

@arunoda arunoda merged commit edfd44c into vercel:canary Mar 6, 2018
@jstcki jstcki deleted the event-source-polyfill branch March 6, 2018 12:18
@jstcki
Copy link
Contributor Author

jstcki commented Mar 6, 2018

Thanks @arunoda @timneutkens 🙏

Another thing: I noticed that next.js uses some features in the framework itself that need to be polyfilled for IE (like Array.prototype.includes) – do you expect users to polyfill those (because you don't want to support IE11)? Would you be open to another PR to either include polyfills for framework code or to replace these features with IE11-compatible code (like indexOf). Personally, I think it would be great if next.js worked out-of-the-box with IE11, so developers only need to polyfill features they actively use in their code.

@arunoda
Copy link
Contributor

arunoda commented Mar 6, 2018

@herrstucki a production Next.js app should work on IE11 without any polyfill. If we use include (or any other ES6 feature), babel will use the correct polyfill when compiling Next.js.
(So no need to add it manually)

@jstcki
Copy link
Contributor Author

jstcki commented Mar 6, 2018

@arunoda you're right. I looked again and saw this only in dev mode in IE11. I traced it back to react-hot-loader where it was already fixed in this commit: gaearon/react-hot-loader@a429374 So if you upgrade RHL to the latest version this should be fixed! 👍

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants