-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Update babel to v7.0.0-beta.31 #24683
Conversation
Hmm I didn't checked babel-eslint 😟 |
Yup, that is the error I was getting when I tried to update too.
EDIT:
Ok I believe it's babel/babel-eslint#530
|
package.json
Outdated
@@ -80,12 +80,12 @@ | |||
"popper.js": "^1.12.6" | |||
}, | |||
"devDependencies": { | |||
"@babel/cli": "7.0.0-beta.5", |
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.
can update to beta.31 now 😛 https://github.com/babel/babel/releases/tag/v7.0.0-beta.31
It's only 5 -> 31 because babylon was at 30
@@ -1,11 +1,11 @@ | |||
module.exports = { | |||
presets: [ | |||
[ | |||
'env', | |||
'@babel/env', |
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.
Is this really needed?
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.
yep that's because the new babel namespace
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.
yeah it's actually because we can't really know to default env
to babel-preset-env
or @babel/preset-env
or @x/preset-env
etc
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.
looks fine, just saying that you can change from beta.5 to beta.31 (it's supposed to be beta.6 but babylon was already at 30 and we moved it into the repo)
@hzoo: we get build errors in babel-eslint; see the Travis build and my comment above. |
package.json
Outdated
@@ -102,7 +102,7 @@ | |||
"qunit-phantomjs-runner": "^2.3.0", | |||
"qunitjs": "^2.4.0", | |||
"rollup": "^0.50.0", | |||
"rollup-plugin-babel": "^3.0.2", | |||
"rollup-plugin-babel": "^4.0.0-beta.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.
Maybe we should remove ^
here.
Ok that's unrelated to babel itself, I haven't looked at babel-eslint in a while actually but yeah maybe due to a newer version of eslint so it needs updates |
The thing is that we don't get any errors if we don't update to beta.5. Something is wrong somewhere :/ |
Oh what, that really doesn't make sense then lol, since eslint is supposed to run on source code not the compiled code (right?), and they don't share dependencies either so that is really weird..🤔 |
1eaf615
to
d116703
Compare
I tried to update to the latest release of ESLint but we still have the same error 😟 https://travis-ci.org/twbs/bootstrap/jobs/297865624#L545 I removed |
aside: curious why the |
If you distribute a build on npm and someone tries to compile node_modules with Babel it will current read it and try to compile with that config which doesn't make sense unless they install your devDeps too (haven't quite figured out what the behavior should be though) |
Still have to investigate but I suppose it's an issue with npm trying to resolve different versions of the beta together? doesn't seem like there is any reason for it do be different because of a babel change |
I believe that previously we allowed this, but it doesn't make sense to me, thus why I changed it in that PR. I expect when I pull a package from npm to either use it as is, or use my scripts for further processing. |
Ok lol so it must be a really weird dependency mismatch? I think I just tested making the dependency scoped and it works.. https://github.com/babel/babel-eslint/blob/2004b913c7abef8d21cebca7f3ae39a9deaee767/package.json#L15 |
try And update do beta.31 instead of beta.5? |
@hzoo: it seems babel-eslint 8.0.2 fixed the issue! Now about beta.31, do we need to add babel/core manually?
|
package.json
Outdated
@@ -35,8 +35,8 @@ | |||
"js-lint": "eslint js/ && eslint --config js/tests/.eslintrc.json --env node build/", | |||
"js-lint-docs": "eslint --config js/tests/.eslintrc.json assets/js/ docs/ sw.js", | |||
"js-compile": "npm-run-all --parallel js-compile-*", | |||
"js-compile-standalone": "cross-env ROLLUP=true rollup --environment BUNDLE:false --config build/rollup.config.js --sourcemap", | |||
"js-compile-bundle": "cross-env ROLLUP=true rollup --environment BUNDLE:true --config build/rollup.config.js --sourcemap", | |||
"js-compile-standalone": "cross-env rollup --environment BUNDLE:false --config build/rollup.config.js --sourcemap", |
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.
Does this mean we can get rid of cross-env
in these 2 lines?
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.
Yep thanks to the removal of external-helpers
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.
Can you remove it where it's no longer needed?
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.
Nope we still use it one line under for js-compile-plugins
instead of if we find a way to avoid the use of cross-env
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.
Yes, but it seems it's redundant in these 2 lines here (not the plugins script
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.
Yep I'll remove it
930e065
to
ad2ebc1
Compare
Right |
7d9761e
to
6c2e95b
Compare
3c12f23
to
e06cf39
Compare
Thanks for all the help, I really appreciate it :)
…On Nov 7, 2017 15:15, "Henry Zhu" ***@***.***> wrote:
Right @babel/core is a peerDep now so yes (so then you don't install v6
stuff)
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#24683 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAVVtUoHkBr-33S3av43LCvK6N7DZg1Jks5s0FfvgaJpZM4QSd_s>
.
|
/CC @hzoo if you have time 😄