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

chore: upgrade core-js to version 3 #25158

Merged
merged 3 commits into from
Jul 1, 2020
Merged

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Jun 20, 2020

Description

Upgrade babel-preset & gatsby to core-js 3. Core-js 2 has been deprecated for a while but we never had a good story to upgrade it. Now with the polyfill work we're doing we can.

I've added a core-js resolver to make sure packages that are still compiled with core-js@2 are still working. It also makes our bundles smaller when core-js@2 is used inside the site itself.

Documentation

Nope

Related Issues

Fixes #15601
Fixes #23254
Addresses #24859

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 20, 2020
@@ -46,7 +45,7 @@ if (semver.prerelease(version)) {
report.warn(
report.stripIndent(`
You are currently using a prerelease version of Node (${version}), which is not supported.
You can use this for testing, but we do not recommend it in production.
You can use this for testing, but we do not recommend it in production.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is prettier

@@ -11,7 +11,6 @@
},
"dependencies": {
"@babel/code-frame": "^7.10.1",
"@babel/runtime": "^7.10.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was unused

@@ -1,6 +1,5 @@
#!/usr/bin/env node

import "@babel/polyfill"
Copy link
Contributor Author

@wardpeet wardpeet Jun 20, 2020

Choose a reason for hiding this comment

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

we don't need this anymore (node 10.13 is pretty good)

@wardpeet wardpeet marked this pull request as ready for review June 20, 2020 16:36
@wardpeet wardpeet requested a review from a team as a code owner June 20, 2020 16:36
@polarathene
Copy link
Contributor

This will still trigger the warning afaik due to a long dependency chain to fbjs which has received no updates for 2 years to npm. There was a recent PR to address the core-js v3 warning there too, however it's only been merged without pushing an update out.

warning gatsby > gatsby-cli > gatsby-recipes > graphql-tools > @graphql-tools/relay-operation-optimizer > relay-compiler > fbjs > [email protected]: core-js@<3 is no longer maintained and not recommended for usage due to the number of issues. Please, upgrade your dependencies to the actual version of core-js@3.

@freiksenet
Copy link
Contributor

freiksenet commented Jun 22, 2020

@polarathene Great catch! I've created an issue for recipes team to handle this. #25184

freiksenet
freiksenet previously approved these changes Jun 22, 2020
@freiksenet freiksenet added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 22, 2020
@wardpeet wardpeet removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jun 22, 2020
@wardpeet
Copy link
Contributor Author

I need to publish a minor for babel-presets cause it's "breaking"

@pieh pieh added breaking change If implemented, this proposed work would break functionality for older versions of Gatsby topic: webpack/babel Webpack or babel labels Jun 23, 2020
@@ -614,7 +615,6 @@ module.exports = async (
`@reach/router/lib/history`,
`@reach/router`,
`common-tags`,
/^core-js\//,
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we want this external anymore? or does the resolver do this somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw this break sometimes with gatsby-dev, never with a regular yarn install, so more or less just a precaution.

Copy link
Contributor

Choose a reason for hiding this comment

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

So should we bring it back then before we ship? My understanding of this array (based on the comment above) is that we force these libs into a common bundle to not bloat page bundles. Doesn't removing this line put core-js in each bundle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we weren't merging core-js into SSR bundles to reduce size. I'll try to add it back and see what tests say.

Copy link
Contributor

@blainekasten blainekasten left a comment

Choose a reason for hiding this comment

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

General approach is looking great! It feels like this could very well break for people with edge cases. I'm wondering if we should wait for v3 on this?

@wardpeet
Copy link
Contributor Author

General approach is looking great! It feels like this could very well break for people with edge cases. I'm wondering if we should wait for v3 on this?

I tested lots of cases haven't find any issues. It can only fail when core-js is used in the root or packages are not configured correctly. This will also become less of an issue when we merge #25159 & #25162.

This improves performance so much and fixes lots of issues with people having libraries that depend on core-js@3. I'm willing to take that shot. We can always revert if I didn't do my homework

@blainekasten
Copy link
Contributor

@wardpeet that sounds good. I'm happy to see this merged if you're ready to own the reverting process if it does raise bad bugs. But I trust you did your homework and the code looks good. Getting core-js 3 will be a big win!

@wardpeet
Copy link
Contributor Author

i'll fix the interop and merge it tomorrow

@wardpeet wardpeet force-pushed the feat/core-js-resolver branch 3 times, most recently from c69e5b6 to 5b2a50a Compare July 1, 2020 10:42
@blainekasten
Copy link
Contributor

@wardpeet do we need to upgrade gatsby-plugin-mdx?

Copy link
Contributor

@blainekasten blainekasten left a comment

Choose a reason for hiding this comment

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

This is phenomenal. Tested locally and things are working great!

@blainekasten blainekasten merged commit a5396ef into master Jul 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the feat/core-js-resolver branch July 1, 2020 14:44
@robbienohra
Copy link

This merge makes me very happy 🥳 🎉

@wadkar
Copy link

wadkar commented Jul 1, 2020

Thanks a lot @wardpeet !! This is great 🥳 🎊

wardpeet added a commit that referenced this pull request Jul 2, 2020
danabrit pushed a commit that referenced this pull request Jul 2, 2020
* chore: upgrade to core-js@3

* add tests to core-js resolver

* update babel-preset tests
@wardpeet wardpeet restored the feat/core-js-resolver branch July 3, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change If implemented, this proposed work would break functionality for older versions of Gatsby topic: webpack/babel Webpack or babel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Can't resolve" importing errors when using canvg Upgrade core-js package to version 3
8 participants