-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Framework: Upgrade to Babel 6 #1515
Conversation
35a33e7
to
e18571c
Compare
Flipping this back to "In Progress", as there appears to be some issues related to https://phabricator.babeljs.io/T6644 Will plan to take a closer look soon, or wait until a proper resolution is put in place in Babel if it indeed turns out to be an issue there. |
The differences between what I was seeing locally compared to CircleCI appear to be due to the fact that I was running a more recent version of |
a3b0d36
to
2913f91
Compare
I've rebased and the tests now run, probably due to Node 4 in #1204 (though we're still on npm 2.x). However, the tests still fail consistently, due to a few slow test suites. I'm unsure yet whether this is an issue with the tests themselves (FeedStore and FeedPostList), or if Babel 6 is actually slower than Babel 5, causing the tests to fall over the timeout. /cc @blowery |
hmmmm. locally:
|
though i'm on npm 3.something... |
I've seen at least one report claiming that Babel can be significantly slower when used with npm 2 than with npm 3. Not obvious to me why this would be the case (related to flattened directly structure?): https://news.ycombinator.com/item?id=10475713 I'll plan to do some more testing early next week. |
este/este@b04f951 might help though if we do that, we should force the docker build to use npm3 too |
Definitely worth testing to see if it helps. I'll commit shortly. |
2913f91
to
092c16e
Compare
rebased this on current master, fixed the conflicts, added the npm3 to see. if npm3 doesn't fix it, we can back it out pretty easily. |
b72da9e
to
15e8fad
Compare
e4a261e
to
364fadd
Compare
Just a quick data point: building |
@@ -2,7 +2,7 @@ NODE_BIN := $(shell npm bin) | |||
MOCHA ?= $(NODE_BIN)/mocha | |||
BASE_DIR := $(NODE_BIN)/../.. | |||
NODE_PATH := test:$(BASE_DIR)/client:$(BASE_DIR)/shared | |||
COMPILERS ?= js:babel/register-without-polyfill |
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 test this just to make sure it didn't break without the polyfill version and it was fine - 24/24 passing
Looks good @aduth - what obstacles remain before merging this? |
The only obstacle was holiday time 😄 I'll respond to your comments shortly. |
Pushed a rebase to resolve merge conflicts. Expected that tests will continue to fail however, pending remaining Remaining on Babel 5 is becoming a bit more painful over time, as plugins we'd like to adopt or create are only available on Babel 6. For example:
|
I merged and deployed all |
@@ -134,30 +131,34 @@ | |||
"version": "1.4.1" | |||
}, | |||
"babel": { | |||
"version": "5.8.12", | |||
"version": "5.8.38", |
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.
Who is bringing this in still...
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.
It's a dependency of wpcom.
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.
huh.
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.
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.
filed Automattic/wpcom.js#200 to try and get it pulled as a dependency
👍 I tested this against a local docker 🐋 image. No visible changes to Calypso. |
@aduth were there any other blockers on this one? The branch needs a rebase, but I think it's really close. |
@gwwar Yeah, it's essentially mergeable. Performance hit is really unfortunate but I don't think we'll be able to stick to Babel 5 long-term, so the hit is inevitable. The only other thing I wanted to check to see was whether there is any impact consideration for other projects that depend on Calypso, such as the desktop app. |
Rebased to resolve merge conflicts. Also made the following changes:
Hope with these changes is (a) limit the number of dependencies, (b) potentially gain some performance boost (though in my anecdotal experience there is no noticeable difference) and (c) avert temptation to use stage 1 features or Flow types. |
This pull request seeks to upgrade Calypso from Babel 5.8.12 to Babel 6.9.x. Babel 6 is a significant departure in its approach, switching to a modular approach. As such, it requires opting in to most syntax conversions.
A significant number of the changed files are a consequence of the splitting of
babel/register
to a separatebabel-register
module. Since most of our tests use the require hook, each of their Makefiles needed to be updated to reflect the updated compiler module.Testing instructions:
Run
make test
from the root directory and verify that all tests pass.Ensure that regressions should be found in navigating the application in the browser.