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

Updating babel to latest version #5097

Closed
wants to merge 3 commits into from
Closed

Conversation

skevy
Copy link
Contributor

@skevy skevy commented Jan 3, 2016

This should happen, given the fact that were several versions old, and many bugs have been fixed since the RN/Babel 6 upgrade.

I'm pretty positive this solves some of the issues around .babelrc's in people's projects not being respected. And by "pretty positive" I mean that in my test project I'm able to use a babelrc and it works just fine (but didn't prior to this upgrade).

@skevy skevy added the Cleanup label Jan 3, 2016
@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @tadeuzagallo, @spicyj and @davidaurelio to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 3, 2016
@satya164
Copy link
Contributor

satya164 commented Jan 3, 2016

How about adding something like http://greenkeeper.io/ ?

@skevy
Copy link
Contributor Author

skevy commented Jan 3, 2016

That would be cool if we were able to continuously keep the Travis build green. 😊

@martinbigio
Copy link
Contributor

I run into #4137 which should be fixed by this. I'll import this test it and land it

@martinbigio
Copy link
Contributor

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/776778005799723/int_phab to review.

@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

Thanks @martinbigio!

@facebook-github-bot
Copy link
Contributor

@skevy updated the pull request.

@skevy
Copy link
Contributor Author

skevy commented Jan 6, 2016

@martinbigio @mkonicek just FYI, I just updated this to lock babel to the minor version (e.g. 6.3.x).

A bug that just broke React Native just landed in babel 6.4.0. Ref: https://phabricator.babeljs.io/T6926 -- and fixed here, but not yet released: babel/babel#3242

So, @ide and I were talking, and we think it's reasonable and prudent to just lock to minor versions on Babel, because it's such a critical piece of the infra.

FYI (simply because I just learned this today...though it's probably something you all already know haha) - shrinkwrap is not respected when you have react-native as a dep...only when you run npm install in the react-native project (for example, with tests...which is why those are still passing).

@skevy
Copy link
Contributor Author

skevy commented Jan 6, 2016

We can update babel to 6.4.x when this bug is resolved and we verify there are no regressions. For now, I think we can stick with 6.3.x

@martinbigio
Copy link
Contributor

@skevy thanks for looking into this! locking on minor versions sounds like a good plan

@ide
Copy link
Contributor

ide commented Jan 7, 2016

+1 to locking minor versions and leaving bugfixes flexible. Babel actually pushed out a breaking change (according to the changelog at least) with the latest minor update so it might make sense to treat Babel's minor version the way we'd normally treat major versions for other packages.

@skevy
Copy link
Contributor Author

skevy commented Jan 7, 2016

What do you guys think about merging this? Currently, until babel fixes their issue, react-native is broken for anyone who installs from npm.

@skevy
Copy link
Contributor Author

skevy commented Jan 7, 2016

Though I suppose they'll probably push a hotfix shortly? Idk.

@skevy
Copy link
Contributor Author

skevy commented Jan 7, 2016

Actually, I lied. This won't actually fix anything, because babel doesn't lock to minors...so even if you install 6.3.26 version of babel-core, it will just install v6.4.0 of babel-generator (which is where the bug is). So, nevermind on any urgency with merging this.

We're stuck with brokenness until babel releases 6.4.1 with a fix for this problem.

crosses fingers that it's soon

@ide
Copy link
Contributor

ide commented Jan 7, 2016

The way we could fix this is to surgically go in and modify the shrinkwrap in order to force it to install 6.3.x versions of babel modules, but it could get really complicated. Waiting for 6.4.1 sounds best.

cc @amasad @sebmck -- could you publish a new version of Babel shortly as a hotfix that includes babel/babel#3245? Currently RN is broken for everyone doing a fresh npm install and I don't see an easy way to work around it from RN's side unfortunately.

EDIT: Thanks, we'll try with 6.4.1 =)

@amasad
Copy link
Contributor

amasad commented Jan 7, 2016

published, sorry about that

@skevy
Copy link
Contributor Author

skevy commented Jan 7, 2016

It's all good @amasad - thanks for being so diligent!

@facebook-github-bot
Copy link
Contributor

@skevy updated the pull request.

@skevy
Copy link
Contributor Author

skevy commented Jan 9, 2016

Superseded by #5214

@skevy skevy closed this Jan 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants