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

Update to Babel 6 #778

Closed
wants to merge 3 commits into from
Closed

Update to Babel 6 #778

wants to merge 3 commits into from

Conversation

psalz
Copy link
Contributor

@psalz psalz commented Jan 6, 2016

I'm aware that there is another pull request for this (#730), but since its humongous, I wanted to share my take on it.

Note that the loose option was removed in Babel 6, so the output is expected to be different. There is an unofficial preset for loose ES2015, but I haven't checked it out yet.

Everything seems to be working as expected, except for the fact that issue #14 has re-emerged somehow (even with the underscore hack). However, running the respective command (e.g. npm run dev) a second time has always fixed it for me so far.

I also changed the way the build command is executed to ensure that everything within prod.config.js is running with NODE_ENV=production. This is for example required for Babel not to include the typecheck plugin within the build.

@quicksnap
Copy link
Collaborator

Thanks! I'm going to check this out--really appreciate it.

That underscore issue is really annoying..

@strawbrary
Copy link
Contributor

Coincidentally, the ApiClient is not defined error should be fixed with the release of Node v5.4.0 today. There was a v8 bugfix they patched in which I think was related.

@quicksnap
Copy link
Collaborator

Do you have a link to info on that bugfix and/or how it relates? I'm curious..

@strawbrary
Copy link
Contributor

@quicksnap: This was the original Babel issue: https://phabricator.babeljs.io/T2455
This issue was filed against v8 as a result: https://bugs.chromium.org/p/v8/issues/detail?id=4450
And this is the commit that landed it in node: nodejs/node@b4c51c5b76

I'm not exactly clear on the whole thing though since the v8 bug is about Unicode but the Babel issue has a repro that doesn't contain special Unicode characters...

@isTravis
Copy link
Contributor

isTravis commented Jan 8, 2016

Thank you! These changes worked for my stack - which has been moderately altered from the beginning point of this boiler plate!

I did have a bunch of syntax that no longer worked though. It was simple enough to fix those - but I'm getting strange run-time bugs now - like Cannot read property 'setState' of undefined when calling this.setState() in my code. Do you know of any docs pointing out babel5>babel6 gotchas?

Edit: this.setState() bug was actually a promises bug because I was behind a few versions on some of my webpack npm modules were out of date (but didn't show up in the PR diff here because they were up to date for y'all). Thanks again! All is looking smooth - really appreciate the migration help!


// Webpack config for development
var fs = require('fs');
var path = require('path');
var webpack = require('webpack');
var WebpackIsomorphicTools = require('webpack-isomorphic-tools');

Choose a reason for hiding this comment

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

hmm why was this removed? Not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, to my understanding the non-plugin version of webpack-isomorphic-tools is only required for server-side patching of the require function, which should be covered here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But as it is not really related, I can squash and move this change as well as the (alphabetical) reordering of npm packages into a separate cleanup commit, should this be approved.

@disbelief
Copy link

I tried out this PR on a clean installation of react-redux-universal-hot-example and it works, but one caveat: Babel 6.4 requires a semi colon after class properties. This required trivial changes (added semicolons) to the following files:

    modified:   src/components/CounterButton/CounterButton.js
    modified:   src/components/InfoBar/InfoBar.js
    modified:   src/components/MiniInfoBar/MiniInfoBar.js
    modified:   src/components/SurveyForm/SurveyForm.js
    modified:   src/containers/About/About.js
    modified:   src/containers/App/App.js
    modified:   src/containers/Chat/Chat.js
    modified:   src/containers/Login/Login.js
    modified:   src/containers/LoginSuccess/LoginSuccess.js
    modified:   src/containers/Survey/Survey.js
    modified:   src/containers/Widgets/Widgets.js
    modified:   src/helpers/Html.js

@flying-sheep
Copy link

i’d rather use babel 6.3 until babel 6.5 (or 6.4.x) reverts that PR (as described in the PR)

@josmardias
Copy link
Contributor

Which one has more chances to be merged, PR #759 or this one?

@quicksnap
Copy link
Collaborator

This one most likely, as it's much smaller.

@flying-sheep
Copy link

OK, 6.5.2 has it reverted!

@@ -124,28 +140,26 @@
},
"devDependencies": {
"autoprefixer-loader": "^3.1.0",
"babel-core": "~5.8.33",
Copy link
Contributor

Choose a reason for hiding this comment

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

why some packages were moved to from devDependencies to dependencies?

@andrewmclagan
Copy link
Collaborator

Any word on this babel 6 migration?

@quicksnap
Copy link
Collaborator

I personally would not like to merge until I've tested Babel 6 on my fork of the project (that is used at work).

If anyone out there is using these changes already, would love to hear feedback. I will probably try to migrate our project to Babel 6 sometime in the next few weeks.

I've spun up a test project and Babel 6 has been playing nicely. So long as the legacy-decorators thing works fine, I don't foresee any big issues 🎱

@strawbrary
Copy link
Contributor

I've been using Babel 6 on my project for a month and it's been mostly smooth sailing. The transform-decorators-legacy plugin works great, the only issue I've run into is that export * from ... syntax is broken in Babel 6.

@disbelief
Copy link

I've been using Babel 6 with my project for a couple weeks now. I was seeing the ReferenceError: ApiClient is not defined bug (#14), but after upgrading node-js to version 5.6.0 (also released this week) this stopped happening as well. Otherwise, everything is working perfectly for me.

@duro
Copy link

duro commented Feb 16, 2016

I'm about to start a new project, I'd love to see this merged.

@duro
Copy link

duro commented Feb 16, 2016

@disbelief I got that same error. Upgrading to node 5 now.

@andrewmclagan
Copy link
Collaborator

I will be giving this a go later in the month.

@ngduc
Copy link

ngduc commented Feb 16, 2016

ditto: would love to see this merged. Thanks.

@duro
Copy link

duro commented Feb 17, 2016

I did a test merge locally of this branch into master, and after some easy conflict resolution, project ran just fine.

@quicksnap
Copy link
Collaborator

@psalz could you resolve the merge conflicts and I'll try to merge it in after giving it a whirl.

@mmahalwy
Copy link

@quicksnap
Copy link
Collaborator

I haven't heard much of @psalz on this PR. If anyone else wants to open up a PR with his changes rebased, we can move forward.

@quicksnap
Copy link
Collaborator

(Not trying to rush you @psalz! Thanks for the work on this PR =)

@josmardias
Copy link
Contributor

@quicksnap #935

quicksnap added a commit that referenced this pull request Feb 18, 2016
@quicksnap
Copy link
Collaborator

Closed by #935. Thanks @psalz and @josmardias!

@quicksnap quicksnap closed this Feb 18, 2016
@andrewmclagan
Copy link
Collaborator

@joshhunt appreciated..!

@psalz
Copy link
Contributor Author

psalz commented Feb 20, 2016

Thanks @josmardias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.