-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
chore(gatsby): add ie polyfill to webpack entry #13241
Conversation
This PR looks really great! It's a bit weird that babel-polyfill isn't polyfilling. Let me dig into that first before proceeding with this PR. |
I guess that's because in SSR building, |
let iePolyfill = false | ||
if ( | ||
supportedBrowsers.includes(`ie 9`) || | ||
supportedBrowsers.includes(`ie 10`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are ie-mob 10
and ie-mob 11
, should we also check for those versions? Personally I don't want to do that
supportedBrowsers.includes(`ie 9`) || | ||
supportedBrowsers.includes(`ie 10`) | ||
) { | ||
iePolyfill = `react-app-polyfill/ie9` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we only need to polyfill Set
Map
Symbol
and other polyfills will be done by babel-loader
, we could create our own polyfill rules instead of using react-app-polyfill
, but I love the best practice from create-react-app
@wardpeet any chance to review? the change is relative slight but would save those who are struggling with IE support, especially the |
Hey, sorry for the late response but we rather not make this change. We are more eager to go for #7064. Which will let us babel transform all our plugins which will let babel-polyfill, polyfill all features. At the moment there is a possible workaround by using https://www.gatsbyjs.org/packages/gatsby-plugin-compile-es6-packages/ and adding react-dom to that list. We want you to know that we certainly want to keep you as a contributor to Gatsby. Thanks again, and we look forward to seeing more PRs from you in the future! 💪 💜 |
@wardpeet as I said, even you add those es6 packages to be compiled by babel loader, it won't solve the problem I pointed out, the BTW, I contributed a lot before but was busy with my own stuffs then. I just received my swags today(I got the coupons long before, but just used them last week), they are so good 😄 |
Let me explain a bit more with IE, I'm using an ES6 package |
@pieh see my comment here #13241 (comment), that PR just make all the packages transpiled by Babel, but still doesn't solve the Symbol issue I described |
Hmm, I didn't try it, but if all packages would be transpiled (including React) - wouldn't @babel/preset-env pick up that it needs to polyfill Symbol and that would happen before react-inspector and then both React and react-inspector would use that polyfilled Symbol? |
#14111 seems to work for react-inspector. Somehow I can't get your example working |
@wardpeet That PR works for |
@wardpeet Thanks so much for you for you patients |
@nihgwu Fix got p in Thank you, for your patience 😉 |
cool, will try, thanks |
Description
This PR adds polyfill for IE according to the
browserlist
config, the current polyfill strategy (useBuiltIns: usage
) works well in most cases, but I encountered with errors only on IE in all my sites built with Gatsby, I digged a lot and found that(my guess) it's caused by the polyfill forSymbol
, theuseBuiltIns: usage
doesn't work asReact
can live withoutSymbol
so it's not polyfilled, but then it's polyfilled if theSymbol
is used in other packages, so we have to add theSymbol
polyfill beforeReact
bootstraps, here is my current solution for my sites https://github.com/Autodesk/react-base-table/blob/master/website/gatsby-node.js#L16Related Issues
Fixes #7003