-
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
[v2] Upgrade Babel from 7.0.0-beta.52 to 7.0.0 stable (fixing Windows tests) #7655
Conversation
Combined with PR #7607, this should fix all failing CI tests. |
Looking good, right now we would probably want 7.0.0 as stable was released yesterday. Other question is - should |
@pieh I've just upgraded to v7 stable - tests seem to be running fine still :) Re: corejs, Babel's options say that we need to change the dependency for packages which use the corejs option, so yes, we should replace the deps in packages which specify |
@pieh I've just done all the dependency replacements, all tests passing locally (and hopefully on CI!) Please could you have a look and let me know if there's anything else which needs fixing? |
I don't think we need to handle the core-js bits in packages since we polyfill the environments anyway. I'd love to see this merged and get a release? this is not blocking a bunch of updates for me :) |
[ | ||
`decorators`, | ||
{ | ||
decoratorsBeforeExport: false, |
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.
maybe this should be true? the goal here is to be as permissive as possible
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.
ok, I'll change this
.babel-preset.js
Outdated
@@ -47,7 +47,7 @@ function preset(context, options = {}) { | |||
{ | |||
// we are only polyfilling the node environment | |||
// so we need to enable the runtime replacements for the browser preset | |||
polyfill: !!browser, | |||
corejs: browser ? 2 : false, |
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.
present env should handle this i believe since it's got the selective polyfilling on already?
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.
This might be the case but I was matching the previous behaviour - shall I remove this? And should I keep the changes in the last commit with the Babel runtime dependencies, or can these be left as just @babel/runtime?
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.
I'd say we want to remove it if it doesn't break anything. The reason they added this was exactly to enable less unnecessary polyfilling so it'd be a win to change, even if it's not the same as previous behavior
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.
Okay, I'll remove it and run through the tests. And shall I undo this commit too? (Tests were passing before I added this)
@jquense I've now removed the corejs option from |
Deploy preview for using-contentful failed. Built with commit c7858fe https://app.netlify.com/sites/using-contentful/deploys/5b859a5ab13fb16c4f811804 |
Deploy preview for image-processing failed. Built with commit c7858fe https://app.netlify.com/sites/image-processing/deploys/5b859a59b13fb16c4f8117fb |
AppVeyor failed because Edit: same reason for the failed Netlify previews |
package.json
Outdated
"@babel/preset-flow": "7.0.0-beta.52", | ||
"@babel/preset-react": "7.0.0-beta.52", | ||
"@babel/runtime": "7.0.0-beta.52", | ||
"@babel/core": "7.0.0", |
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.
we should add back the ^
on the versions here since we are out of beta's and there will be no breaking changes between v7 versions
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.
👍
@@ -3,8 +3,8 @@ | |||
"version": "2.0.2-rc.0", | |||
"author": "Jason Quense <[email protected]>", | |||
"devDependencies": { | |||
"@babel/cli": "7.0.0-beta.52", | |||
"@babel/core": "7.0.0-beta.52" | |||
"@babel/cli": "7.0.0", |
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.
ditto for all the babel deps actually
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.
LGTM apart from the needed caret in the version ranges
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.
Nice work!
package.json
Outdated
"@babel/preset-flow": "7.0.0-beta.52", | ||
"@babel/preset-react": "7.0.0-beta.52", | ||
"@babel/runtime": "7.0.0-beta.52", | ||
"@babel/core": "7.0.0", |
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.
👍
[ | ||
`decorators`, | ||
{ | ||
decoratorsBeforeExport: true, |
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.
are these required options now?
@KyleAMathews yep, those are required now. @jquense I'll add the ranged versions back later today and then hopefully this can be merged! |
I've now added the caret ranges! |
packages/gatsby-link/package.json
Outdated
@@ -7,15 +7,16 @@ | |||
"url": "https://github.com/gatsbyjs/gatsby/issues" | |||
}, | |||
"dependencies": { | |||
"@babel/runtime": "7.0.0-beta.52", | |||
"@babel/runtime": "^7.0.0", | |||
"@babel/runtime-corejs2": "^7.0.0", |
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.
This is not needed anymore, right?
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.
Ah yep, I'll just remove that line
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.
Thanks for your usual through/excellent work @davidbailey00!
… tests) (gatsbyjs#7655) * Upgrade to Babel 7.0.0-rc.3 * Update config options and snapshots * Fix tests on Windows * Fix React Babel preset and update snapshots * Upgrade Babel to 7.0.0 (stable) * Allow decorators before export * Remove unnecessary polyfill * Use caret ranges for Babel deps * Remove unneeded dependency
Changelog:
--ignore XYZ
wasn't always working properly on Windows so extra failing tests were being generatedgatsby/cache-dir/__tests__/.babelrc
to match API changes.babel-preset.js
babel-plugin-remove-graphql-queries/src/__tests__/index.js
to match API changesgatsby/src/utils/babel-parse-to-ast.js
to match API changes\n
line endings ingatsby-codemods/src/transforms/global-graphql-calls.js
to fix the remaining Windows tests failingFixes #7652