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

Upgrade to Babel6 #1123

Merged
merged 1 commit into from
Feb 2, 2016
Merged

Upgrade to Babel6 #1123

merged 1 commit into from
Feb 2, 2016

Conversation

pr1ntr
Copy link

@pr1ntr pr1ntr commented Dec 10, 2015

This is my attempt to fix this issue on facebook/react-native#4674.
Upgraded to babel6 builds and tests run successfully.

@@ -12,7 +12,7 @@
"scripts": {
"clean": "rimraf lib dist coverage",
"lint": "eslint src test examples",
"test": "mocha --compilers js:babel/register --recursive",
"test": "mocha --compilers js:babel-core/register --recursive",
Copy link

Choose a reason for hiding this comment

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

The recommended setup now: https://babeljs.io/docs/setup/#mocha

npm install --save-dev babel-register

and

"test": "mocha --compilers js:babel-register"

@pr1ntr
Copy link
Author

pr1ntr commented Dec 10, 2015

i'm on it

@just-boris
Copy link
Contributor

You should update package.json and .babelrc in all examples too to make build passed

@pr1ntr
Copy link
Author

pr1ntr commented Dec 10, 2015

@just-boris, yeah I realize now. May take a little time. There is also the babel-polyfill issue

@pr1ntr
Copy link
Author

pr1ntr commented Dec 10, 2015

todos reducer
    ✓ should handle initial state
    ✓ should handle ADD_TODO
    ✓ should handle DELETE_TODO
    ✓ should handle EDIT_TODO
    ✓ should handle COMPLETE_TODO
    ✓ should handle COMPLETE_ALL
    ✓ should handle CLEAR_COMPLETED
    1) should not generate duplicate ids after CLEAR_COMPLETED


  49 passing (99ms)
  1 failing

  1) todos reducer should not generate duplicate ids after CLEAR_COMPLETED:

      Error: Expected [ { completed: false, id: 2, text: 'Write more tests' }, { completed: false, id: 1, text: 'Write tests' } ] to equal [ { completed: false, id: 2, text: 'Write more tests' }, { completed: false, id: 1, text: 'Write tests' } ]
      + expected - actual


      at Object.assert [as default] (node_modules/expect/lib/assert.js:20:9)
      at Expectation.toEqual (node_modules/expect/lib/Expectation.js:69:26)
      at Context.<anonymous> (todos.spec.js:273:7)


I am not sure that this test would pass if i didn't update babel.

@pr1ntr
Copy link
Author

pr1ntr commented Dec 10, 2015

So all the builds pass. it seems that the example's tests are failing. This appears to be a problem that is currently merged into master.

@just-boris
Copy link
Contributor

Here is a build of master and it is failing: https://travis-ci.org/rackt/redux/jobs/96114270

@pr1ntr
Copy link
Author

pr1ntr commented Dec 10, 2015

is this something that I need to fix before this PR is accepted? Personally, I think that test is unnecessary since normally an application like that would be storing data on a DB that has auto-increment.

@ellbee
Copy link
Contributor

ellbee commented Dec 10, 2015

I think the build is failing due to one of our testing dependencies. It's been reported and they are looking into it.

@pr1ntr
Copy link
Author

pr1ntr commented Dec 10, 2015

@ellbee
Copy link
Contributor

ellbee commented Dec 10, 2015

Yep, thats the one. This should start passing again without any changes needed on our end when the dependency is fixed (which seems like it is going to be soon).

@christopherdro
Copy link

🚀

@pr1ntr
Copy link
Author

pr1ntr commented Dec 11, 2015

would like to get this merged too:
https://github.com/rackt/react-redux/pull/212/commits

@ellbee
Copy link
Contributor

ellbee commented Dec 11, 2015

@gaearon Is there any reason I should hold off on this? I will have some time to test it out over the weekend.

@gaearon
Copy link
Contributor

gaearon commented Dec 11, 2015

Proceed by all means!

@ellbee
Copy link
Contributor

ellbee commented Dec 12, 2015

@gaearon When you get a moment...

I am hoping to get Redux, React-Redux and Redux-Devtools ready to go at the same time. These PRs constitute a breaking change I think, so with regards to versioning...

  • Redux: Should be v5.0.0
  • React-Redux: Again v5.0.0, but what about the React Native people stuck with the v3.0.0 branch? It would be nice to release a fix for them but how do we version it?
  • Redux-Devtools: Another beta? A v3 release candidate? v3 proper?

@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2015

What's a breaking change? Version of Babel we use internally shouldn't matter.

@ellbee
Copy link
Contributor

ellbee commented Dec 12, 2015

Ah ok, I think I've got it now. I assumed from the original comment that this PR is necessary to fix the issue with React Native 0.16 (which did seem weird), but it seems like that is not the case—if we simply remove the .babelrc from the current npm package it will build with Babel 5 or 6. That has already been done and will be going out in the next release, so we won't be breaking anyone who is still using Babel 5.

So everything is fine, and I can stop worrying.

@gaearon
Copy link
Contributor

gaearon commented Dec 12, 2015

Yep! I just released redux@3.0.5 without .babelrc and will do the same for react-redux (3.x and 4.x). Though if you could take a look at updating the README for DevTools v3 so we could release it, that would be swell.

@ellbee
Copy link
Contributor

ellbee commented Dec 12, 2015

Cool, sounds good.

@pr1ntr
Copy link
Author

pr1ntr commented Dec 13, 2015

Couldn't hurt to upgrade to Babel 6 anyways yeah?

@gaearon
Copy link
Contributor

gaearon commented Dec 13, 2015

Sure.

}


}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the blank lines that have been introduced into the package.json files in the examples please?

Copy link
Member

Choose a reason for hiding this comment

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

Please leave the extra newline at the bottom of the file. It's really helpful when cat'ing out or doing other command line things so that you don't have weird shell overlaps. Also, it's POSIX standard, believe it or not.

@ellbee
Copy link
Contributor

ellbee commented Dec 13, 2015

@pr1ntr Just went through all the examples and its mostly looking good!

The Universal example is broken. Babel 5 is in the dependencies and babel/register is being required in server/index.js, but I've not looked into it any further than that.

Also, isparta needs to be upgraded to v4.0.0 in the main package.json so the test coverage still works.

@pr1ntr
Copy link
Author

pr1ntr commented Dec 13, 2015

regarding the universal example. I couldn't get that to work with Babel 5 or 6. Are you sure its working on the main branch?

[edit] it totally does.. I'll look into it.

@pr1ntr
Copy link
Author

pr1ntr commented Dec 14, 2015

i updated isparta, and i got universal running. One thing about universal however is that the webpack config doesn't use HMR when running from the repo (../../src exists) I did my best to set it up with babel6 the same way it was with babel5. Let me know if its still not right.

@martincik
Copy link

👍 for merge.

@ellbee
Copy link
Contributor

ellbee commented Dec 17, 2015

@pr1ntr Thanks for updating. I'm super busy, so I don't have any time to look at this right now. I'll have some spare time at the start of next week, so if no-one beats me to it I'll look to get it merged then.

@pr1ntr
Copy link
Author

pr1ntr commented Dec 18, 2015

no problemo!

@pr1ntr
Copy link
Author

pr1ntr commented Dec 20, 2015

fixed whitespace

}


}
Copy link
Member

Choose a reason for hiding this comment

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

Missed one! Sorry for the nitpick :person_frowning:

Copy link
Author

Choose a reason for hiding this comment

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

oops. fixed

@gaearon gaearon changed the title Upgrade to Babel6 [DON'T MERGE YET] Upgrade to Babel6 Dec 28, 2015
@gaearon
Copy link
Contributor

gaearon commented Dec 28, 2015

Note: please don't merge this yet.

We're having some problems with Babel behavior: it no longer includes ES3 transforms necessary for IE8 by default, and there seems to be a bug with those transforms which breaks them anyway.

Because of this we can't ship Redux that uses Babel 6 internally until either

  • that bug is fixed and we add ES3 transforms to the pipeline
  • or we increment major and tell consumers to ES3ify code on their end

@dmr
Copy link

dmr commented Feb 1, 2016

The examples still use babel5 also. I really like these as "boilerplate" but I always have to manually update babel dependencies --> Can we update the examples and leave only main redux with babel5?

@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2016

I really like these as "boilerplate" but I always have to manually update babel dependencies

The examples were never meant as a boilerplate. They are meant as Redux usage examples. I understand people will use them as boilerplate but this is a shame because they lack important aspects such as compiling for production. Having two different Babel versions in one repo would be confusing.

@timdorr timdorr mentioned this pull request Feb 1, 2016
8 tasks
@gaearon gaearon mentioned this pull request Feb 2, 2016
@gaearon gaearon merged commit e3fcdce into reduxjs:master Feb 2, 2016
@gaearon
Copy link
Contributor

gaearon commented Feb 2, 2016

This is now in. Thanks again!

@gaearon gaearon changed the title [DON'T MERGE YET] Upgrade to Babel6 Upgrade to Babel6 Feb 2, 2016
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.

9 participants