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

Framework: Upgrade to Babel 6 #1515

Merged
merged 3 commits into from Jun 9, 2016
Merged

Framework: Upgrade to Babel 6 #1515

merged 3 commits into from Jun 9, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Dec 11, 2015

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 separate babel-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.

@aduth aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 11, 2015
@aduth
Copy link
Contributor Author

aduth commented Dec 11, 2015

Flipping this back to "In Progress", as there appears to be some issues related to babel-runtime which I'm not able to reproduce locally, but are causing test failures in CircleCI. Adding babel-runtime as a dependency results in a different error which looks very similar to the one reported here:

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.

@aduth aduth added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 11, 2015
@aduth
Copy link
Contributor Author

aduth commented Dec 11, 2015

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 node / npm. In which case, upgrading our Node dependency in #1204 could be a solution, though I'll see if there's another interim option.

@aduth aduth force-pushed the update/babel-6 branch 2 times, most recently from a3b0d36 to 2913f91 Compare December 18, 2015 22:04
@aduth
Copy link
Contributor Author

aduth commented Dec 18, 2015

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

@blowery
Copy link
Contributor

blowery commented Dec 19, 2015

hmmmm. locally:

FeedStore
    ✓ should have a dispatch token
    ✓ should not contain an unknown feed ID
    ✓ should create a pending record when sent REFRESH
    ✓ should pass through the pending state and prevent double fetches
    ✓ should accept a good response
    ✓ should accept an error


  6 passing (664ms)



  FeedPostList
    ✓ should require an id, a fetcher, a keyMaker
    A valid instance
      ✓ should receive a page
      ✓ should receive updates
      ✓ should treat each set of updates as definitive
    Selected index
      ✓ should initially have nothing selected
      ✓ should select the next item
      ✓ should select the next valid post
      ✓ should be able to select a gap
      ✓ should select the prev item
      ✓ should select the prev valid post
    Filter followed x-posts
      ✓ rolls up x-posts and matching x-comments
      ✓ when following origin site, filters followed x-posts, but leaves comment notices
      ✓ updates sites x-posted to
      ✓ filters xposts with no metadata
      ✓ filters xposts with missing xpost metadata


  15 passing (938ms)

@blowery
Copy link
Contributor

blowery commented Dec 19, 2015

though i'm on npm 3.something...

@aduth
Copy link
Contributor Author

aduth commented Dec 19, 2015

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.

@blowery
Copy link
Contributor

blowery commented Dec 19, 2015

este/este@b04f951 might help

though if we do that, we should force the docker build to use npm3 too

@aduth
Copy link
Contributor Author

aduth commented Dec 19, 2015

este/este@b04f951 might help

Definitely worth testing to see if it helps. I'll commit shortly.

@blowery
Copy link
Contributor

blowery commented Dec 19, 2015

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.

@blowery
Copy link
Contributor

blowery commented Dec 22, 2015

Just a quick data point: building make build-wpcalypso takes 40s on both babel 5 (40.2s) and babel 6 (40.4s).

@aduth aduth added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 22, 2015
@@ -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
Copy link
Contributor

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

@dmsnell
Copy link
Contributor

dmsnell commented Dec 28, 2015

Looks good @aduth - what obstacles remain before merging this?

@dmsnell dmsnell added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 28, 2015
@aduth
Copy link
Contributor Author

aduth commented Jan 4, 2016

what obstacles remain before merging this?

The only obstacle was holiday time 😄 I'll respond to your comments shortly.

@aduth
Copy link
Contributor Author

aduth commented Jun 5, 2016

Pushed a rebase to resolve merge conflicts. Expected that tests will continue to fail however, pending remaining rewire dependencies to be removed (#5810, #5809 #5811).

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:

  • babel-plugin-transform-react-remove-prop-types to remove propTypes from production builds
  • Pending changes to state selectors to import from single file import { getSelectedSiteId } from 'state/selectors'; could be optimized with a Babel plugin to avoid unnecessary importing without need for tree shaking from Webpack 2 (gist)

@gziolo
Copy link
Member

gziolo commented Jun 6, 2016

I merged and deployed all rewire related PRs. Great work on that one @aduth :)
I removed no longer used rewire dependency and regenerated shrinkwrap config again. I had also to resolve two more issues related to Babel upgrade and unit tests. It looks like unit tests are all green now 🎉 🎈
It looks like the tests are a bit slower with the latests Babel version, but I wouldn't consider it as a blocker.
I also checked couple of pages and they work as before.

@gziolo gziolo added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 6, 2016
@@ -134,30 +131,34 @@
"version": "1.4.1"
},
"babel": {
"version": "5.8.12",
"version": "5.8.38",
Copy link
Contributor

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...

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

huh.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

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

@gwwar
Copy link
Contributor

gwwar commented Jun 6, 2016

👍 I tested this against a local docker 🐋 image. No visible changes to Calypso.

@gwwar
Copy link
Contributor

gwwar commented Jun 8, 2016

@aduth were there any other blockers on this one? The branch needs a rebase, but I think it's really close.

@aduth
Copy link
Contributor Author

aduth commented Jun 8, 2016

@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.

@aduth
Copy link
Contributor Author

aduth commented Jun 9, 2016

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.

@gwwar
Copy link
Contributor

gwwar commented Jun 9, 2016

:shipit:

@gwwar gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 9, 2016
@aduth aduth merged commit cf5441e into master Jun 9, 2016
@aduth aduth deleted the update/babel-6 branch June 9, 2016 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants