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

Map and Set Polyfill clarification #16945

Closed
chrisdl opened this issue Aug 21, 2019 · 13 comments · Fixed by #18121
Closed

Map and Set Polyfill clarification #16945

chrisdl opened this issue Aug 21, 2019 · 13 comments · Fixed by #18121
Assignees
Labels
good first issue Issue that doesn't require previous experience with Gatsby help wanted Issue with a clear description that the community can help with. type: documentation An issue or pull request for improving or updating Gatsby's documentation

Comments

@chrisdl
Copy link

chrisdl commented Aug 21, 2019

Hey

I was reading the documentation:

https://www.gatsbyjs.org/docs/browser-support/#note-about-ie--11

And it has a note about IE < 11, if I am understanding it correctly I should install core-js and add

import 'core-js/es/map';
import 'core-js/es/set';

Somewhere.

Do I need to do this if I want to support IE 11 and perhaps 10? Or is this handled by Gatsby? If I do add these imports, where would I do so? gatsby-browser.js?

I've also edited by browserslist:

"browserslist": [
    ">0.8%",
    "ie >= 11",
    "not op_mini all",
    "not dead"
  ]

I just feel like the docs are a little unclear about the actual method for adding polyfills for browser support.

Thanks!

@onestopjs
Copy link
Contributor

I think importing the polyfills in gatsby-browser.js will do the job. I haven't tested it though, so it would be great if you can test it and share the results.

You are right the docs are a bit unclear, although I am not sure where it is best to be mentioned.

@LekoArts LekoArts added the type: documentation An issue or pull request for improving or updating Gatsby's documentation label Sep 4, 2019
@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Sep 25, 2019
@gatsbot
Copy link

gatsbot bot commented Sep 25, 2019

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.

If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@mendaomn
Copy link
Contributor

Would love to help with this. I'll test #16945 (comment) and report back!

@mendaomn
Copy link
Contributor

mendaomn commented Oct 3, 2019

So here's what I've found:

Set and Map seem to be polyfilled

The default Gatsby project (gatsby new my-project) does seem to include Set and Map polyfills. To be more precise, I've found in the generated bundle the same lines of code exported by core-js/es/set

For reference, here's the code included in the generated bundle:

n(73)("Set", (function (t) {
  return function () {
    return t(this, arguments.length > 0 ? arguments[0] : void 0)
  }
}), {
  add: function (t) {
    return r.def(o(this, "Set"), t = 0 === t ? 0 : t, t)
  }
}, r)

Gatsby should add polyfills for your target browsers

The docs specify that:

Gatsby leverages Babel 7’s ability to automatically add polyfills for your target browsers.

More related to this issue, they also make the following example (emphasis mine):

If you start using a newer JavaScript API like [].includes that isn’t supported by some of your targeted browsers, you won’t have to worry about it breaking the older browsers as Babel will automatically add the needed polyfill core-js/modules/es7.array.includes.


In light of this, I would suggest that we remove the part in the docs that suggests to polyfill Map and Set in order to support IE < 11.

It still isn't all that clear how to manually add polyfills to the bundle, as according to the docs gatsby-browser.js should be used to interact with Gatsby's browser APIs.

Personally, I would try the wrapPageElement API and wrap every page in a Component that includes the polyfill, but it's a weird solution and I suppose there's a better way!

@chrisdl
Copy link
Author

chrisdl commented Oct 3, 2019

Thanks for doing the leg work @mendaomn so babel (7 and up) ended up doing the polyfilling. 👍

Want me to close this thread now @gatsbybot ? =)

@mendaomn
Copy link
Contributor

mendaomn commented Oct 4, 2019

No problem! What do you think about removing the part in the docs that says you should polyfill Map and Set yourself?

As you experienced, it might lead to confusion I think :)

@gillkyle
Copy link
Contributor

gillkyle commented Oct 4, 2019

Thanks for the research on this! It looks like changes were merged that would handle transpilation of node_modules here. I think that aligns with @mendaomn's findings and I'm guessing the commit adding supporting those polyfills is an artifact of the note in the React docs about supporting older browsers that might be necessary in React if you aren't using something like Gatsby or create-react-app.

If someone would like to take this on and remove that from the docs, and open a PR it'd be much appreciated 🙂 I'll mark this as Hacktoberfest and Help Wanted, if one of you would like to claim it wel can mark it as Hacktoberfest - Claimed

@gillkyle gillkyle added good first issue Issue that doesn't require previous experience with Gatsby Hacktoberfest help wanted Issue with a clear description that the community can help with. and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Oct 4, 2019
@jbutz
Copy link
Contributor

jbutz commented Oct 4, 2019

@gillkyle I've got the time to knock this out today if no one else has claimed it.

@gillkyle
Copy link
Contributor

gillkyle commented Oct 4, 2019

Awesome thanks @jbutz! I see your commit referenced this issue but I don't see a PR yet, feel free to create one and we can get this merged 🙂

@jbutz
Copy link
Contributor

jbutz commented Oct 4, 2019

@gillkyle I threw together the code changes since they were so small, but wanted to give people a chance to respond. No sense stealing Hacktoberfest credit away from people who reported an issue or did the research. 😉

PR is now open

@gillkyle
Copy link
Contributor

gillkyle commented Oct 4, 2019

Yeah good point @jbutz, thanks for being so courteous, since I just barely added the Hacktoberfest label today I'll merge the PR.

If anyone else is in this thread is interested in getting something merged for Hacktoberfest I'd be happy to pair with you and get something merged into Gatsby 🙂

@mendaomn
Copy link
Contributor

mendaomn commented Oct 4, 2019

That would be awesome! I'd love to contribute to Gatsby @gillkyle

@gillkyle
Copy link
Contributor

gillkyle commented Oct 8, 2019

Hey @mendaomn I just put up another issue in #18329 that I think is a really low barrier to entry addition you could make if you'd like, I marked it as claimed already, but if you don't feel like you'll be able to get to it I can open it back up to the rest of the community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issue that doesn't require previous experience with Gatsby help wanted Issue with a clear description that the community can help with. type: documentation An issue or pull request for improving or updating Gatsby's documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants