Rollup #994
Rollup #994
Conversation
.babelrc
Outdated
| ], | ||
| "presets": [ | ||
| "env", | ||
| ["env", { "modules": false }], |
There was a problem hiding this comment.
I'd recommend to refactor this whole file. I usually just create .babelrc.js for better control (you have to put {"presets": ["./.babelrc.js"]} in you .babelrc so babel6 can handle it) and then I can just use javascript to construct plugins/presets based on process.env variables. It's imho hard to see what changes between various variants here, because of overwhelming duplication
There was a problem hiding this comment.
Oh, I'd also recommend using loose mode with modules: false in call cases + smth like
const cjs = process.env.NODE_ENV === 'test' || process.env.BABEL_ENV === 'cjs'
// ...
plugins: [
// ...
cjs && 'transform-es2015-modules-commonjs'
].filter(Boolean)Why? loose mode for everything except commonjs transform, because it makes __esModule property enumerable, so I consider default mode better for this case.
rollup.config.js
Outdated
| }), | ||
| babel({ | ||
| exclude: 'node_modules/**', | ||
| runtimeHelpers: true, |
There was a problem hiding this comment.
would be good to compare actual output for this and for the variant with plugins: ['external-helpers'] in case of umd build, I suspect that latter might be slightly better or there is no difference - im curious which one of those 2 😅
| exclude: 'node_modules/**', | ||
| runtimeHelpers: true, | ||
| }), | ||
| uglify({ |
There was a problem hiding this comment.
I think you are lacking a rollup-plugin-replace here which would replace process.env.NODE_ENV in your bundle
|
Good thinking with using rollup for just umd build first 👌 |
|
😮 ... 🎉 😎 |
Codecov Report
@@ Coverage Diff @@
## master #994 +/- ##
=======================================
Coverage 90.13% 90.13%
=======================================
Files 59 59
Lines 1653 1653
=======================================
Hits 1490 1490
Misses 163 163Continue to review full report at Codecov.
|
|
@Andarist could you take a look? |
| "build:es": "npm run clean:es && npm run build:types && cross-env NODE_ENV=production cross-env BABEL_ENV=es babel source --out-dir dist/es", | ||
| "build:umd": "npm run clean:umd && cross-env NODE_ENV=production webpack --config webpack.config.umd.js --bail", | ||
| "build:es": "npm run clean:es && npm run build:types && cross-env NODE_ENV=es babel source --out-dir dist/es", | ||
| "build:umd": "npm run clean:umd && cross-env NODE_ENV=rollup rollup -c", |
There was a problem hiding this comment.
i'd just use separate env var for this, let's keep NODE_ENV restricted to things like production, development, test etc
.babelrc.js
Outdated
| 'source/TestUtils.js', | ||
| ]; | ||
|
|
||
| if (env === 'commonjs') { |
There was a problem hiding this comment.
personally I just construct a single module.exports based on env vars, so i just add plugins, presets (or modify some options) conditionally
that way I better see which plugins are common to all possible values and which ones differ
but whatever works for you :)
| if (env === 'rollup') { | ||
| module.exports = { | ||
| comments: false, | ||
| plugins: ['external-helpers'], |
There was a problem hiding this comment.
technically this could be passed to rollup-plugin-babel and it would just merge it with what this .babelrc.js returns, that way you wouldn't have introduce separate env var just for rollup, but u'd somewhat loose 'centralization' of all babel configuration
i wouldnt concern much about the latter as its kinda temporary, because with babel@7 and its corresponding version of the babel plugin you won't have to deal with external-helpers at all, this optimization will be applies automatically by the plugin
There was a problem hiding this comment.
Yeah, I'm gonna open PR with babel7 later
ghost
mentioned this pull request
/cc @Andarist