This repository has been archived by the owner. It is now read-only.

Use rollup for bundling to speed up startup time #190

Merged
merged 2 commits into from Oct 26, 2016

Conversation

Projects
None yet
5 participants
@DrewML
Copy link
Member

DrewML commented Oct 16, 2016

This closes #181.

Copy/pasting the example I posted in that issue:

Test case

const babylon = require('babylon');
const ast = babylon.parse('var foo = "lol";');

Without bundling
image

With bundling
image

With that test case, there was a ~95ms savings by removing the need for node to build/traverse the dependency graph.

As noted in that thread, this requires (for now) dropping npm run watch due to bugs in rollup-watch. Would like to hear feedback from other contributors to determine if this is going to severely impact anyone's workflow.

@@ -29,13 +29,16 @@
"flow-bin": "^0.33.0",
"lodash": "^4.15.0",
"nyc": "^8.1.0",
"rollup": "^0.36.3",
"rollup-plugin-babel": "^2.6.1",
"rollup-plugin-node-resolve": "^2.0.0",

This comment has been minimized.

@DrewML

DrewML Oct 16, 2016

Author Member

This is required to be able to import an index file without having to explicitly list ./foo/index.

if (!pluginMap[name]) {
pluginMap[name] = true;

let plugin = exports.plugins[name];
let plugin = plugins[name];

This comment has been minimized.

@DrewML

DrewML Oct 16, 2016

Author Member

I had to rename the plugins identifier in the argument list for loadPlugins, because it was shadowing the identifier of the same name at the top of the file. The current code is relying on the way the module eventually gets compiled to cjs, but this doesn't work now that we're bundling.

@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Oct 16, 2016

Hmm. Apparently rollup only supports node 0.12.x and up. So our output bundle would work on 0.10, but we can't execute the build in Travis for 0.10 (without using something like babel-register for that version).

Thoughts?

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 17, 2016

We could just not run rollup in node 0.10 and do what we were doing before? We should be able to run the tests with/without rollup right? (might be painful)

@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Oct 17, 2016

We should be able to run the tests with/without rollup right? (might be painful)

My biggest concern there is that, Rollup could theoretically generate a bundle that wouldn't work on 0.10.x, but we wouldn't know if we didn't test the bundle itself.

I did have a thought, though. nvm is available on Travis containers. I could have it setup so, for 0.10/0.12, we run the rollup build in node 4, then nvm use before the tests run. I'll give it a shot later tonight

@wtgtybhertgeghgtwtg

This comment has been minimized.

Copy link

wtgtybhertgeghgtwtg commented Oct 19, 2016

I doubt this is a feasible suggestion, but since node is dropping support for 0.10 at the end of the month, maybe you could bump Babylon to 7 and make it for node 0.12 and up? Alternatively, for node 4 and up, since node drops support for 0.12 at the end of the year.

Although, bumping the minimum node version to 4 seems to be on Babel's Next Major milestone.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 21, 2016

We can just use node 6 to build with rollup and then test on the rest?

@danez

This comment has been minimized.

Copy link
Member

danez commented Oct 21, 2016

agree, maybe even the latest node version

edit: Uppps wrong button

@danez danez closed this Oct 21, 2016

@danez danez reopened this Oct 21, 2016

@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Oct 21, 2016

Cool - I'll push updates to the PR this weekend to build with node latest, then run tests on the target node version

@DrewML DrewML force-pushed the DrewML:rollup branch 3 times, most recently from e2fc2b4 to f4ec01d Oct 23, 2016

@DrewML DrewML force-pushed the DrewML:rollup branch from f4ec01d to 6222bc5 Oct 23, 2016

@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Oct 23, 2016

@hzoo @danez Switched things up in travis so builds run in latest node, and then tests are executed in the proper node version for that test run.

Can verify this in the Travis build logs:

image

"coverage": "nyc report --reporter=json && codecov -f coverage/coverage-final.json",
"lint": "eslint src bin",
"flow": "flow",
"prepublish": "cross-env BABEL_ENV=production npm run build",
"preversion": "npm run test && npm run changelog",
"test": "npm run lint && npm run flow && npm run build && npm run test-only",
"test-only": "ava test",
"test-ci": "cross-env BABEL_ENV=test npm run build && nyc npm run test-only",
"watch": "babel src --out-dir lib --watch",

This comment has been minimized.

@hzoo

hzoo Oct 24, 2016

Member

do we have to remove watch if it doesn't need to use rollup?

This comment has been minimized.

@DrewML

DrewML Oct 24, 2016

Author Member

Hmm, you bring up a good point. I just noticed that I didn't change the test-only command to run a build first, so technically the code you test against locally isn't 1:1 with the code tested in CI (babel-ified code locally, vs rollup output in CI). Thoughts?

This comment has been minimized.

@hzoo

hzoo Oct 24, 2016

Member

Not sure - I don't think we do in Babel either?

This comment has been minimized.

@DrewML

DrewML Oct 24, 2016

Author Member

Ha, that's a good point. We could leave it in, and then, if enough of us hit weirdness where things pass in CI but fail locally, we can always pull it out (or try and fix the rollup watch plugin).

This comment has been minimized.

@danez

danez Oct 25, 2016

Member

I never used watch in babylon as the tests usually do a build before and the build does not take too long.

@hzoo

hzoo approved these changes Oct 24, 2016

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 25, 2016

The 'classCallCheck' Babel helper is used more than once in your code. It's strongly recommended that you use the "external-helpers" plugin or the "es2015-rollup" preset. See https://github.com/rollup/rollup-plugin-babel#configuring-babel for more information

Since we already use modules: false I guess we can add "plugins": [ "external-helpers" ]

https://gist.github.com/hzoo/6f393d0a24be191e25dee0bb894bb09e

@danez
Copy link
Member

danez left a comment

Codecoverage is broken see comments.

.travis.yml Outdated

before_script:
- 'if [ -n "${BABEL-}" ]; then make bootstrap-babel ; fi'
- 'npm run build'

This comment has been minimized.

@danez

danez Oct 25, 2016

Member

We need to add BABEL_ENV=test here infront of the call so that babel-plugin-istanbul is used.

"coverage": "nyc report --reporter=json && codecov -f coverage/coverage-final.json",
"lint": "eslint src bin",
"flow": "flow",
"prepublish": "cross-env BABEL_ENV=production npm run build",
"preversion": "npm run test && npm run changelog",
"test": "npm run lint && npm run flow && npm run build && npm run test-only",
"test-only": "ava test",
"test-ci": "cross-env BABEL_ENV=test npm run build && nyc npm run test-only",
"watch": "babel src --out-dir lib --watch",
"test-ci": "cross-env BABEL_ENV=test nyc npm run test-only",

This comment has been minimized.

@danez

danez Oct 25, 2016

Member

We can remove cross-env BABEL_ENV=test here then as it is only used for babel but babel is not run here.

before_install:
# Rollup doesn't support node < 4.x. Switch to latest for build
- . $HOME/.nvm/nvm.sh
- nvm install stable && nvm use stable

This comment has been minimized.

@danez

danez Oct 25, 2016

Member

maybe should we use nvm install --lts instead of nvm install stable stable is deprecated first of all and is now an alias for node and second it currently installs node 7 and not sure how stable that is already. Or do you think thats okay?

And btw install also automatically calls use.

@danez

danez approved these changes Oct 25, 2016

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 26, 2016

🎉

We can have a separate issue to use external-helpers - maybe it's a babel issue or a rollup one not sure but it's including the newly added asyncGenerator helper for some reason which makes it bigger https://gist.github.com/hzoo/6f393d0a24be191e25dee0bb894bb09e

Wonder how hard it would be to just dedupe it in a plugin instead

@hzoo hzoo merged commit beb8db6 into babel:master Oct 26, 2016

3 checks passed

codecov/patch 100% of diff hit (target 98.09%)
Details
codecov/project Absolute coverage decreased by -1.88% but relative coverage increased by +1.90% compared to 930cdd0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@DrewML DrewML deleted the DrewML:rollup branch Oct 26, 2016

@kittens

This comment has been minimized.

Copy link
Contributor

kittens commented Oct 26, 2016

Yay, this will also stop people doing dumb shit like this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.