-
Notifications
You must be signed in to change notification settings - Fork 188
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
Build with rollup instead of webpack #281
Conversation
Looks like CI is failing because of dropped coverage for some reason. I'm guessing something about the babel update. I'll wait for feedback on this before I look into fixing that. |
Ok, I think I fixed the coverage. Turns out I needed to bring in the istanbul babel plugin so that the coverage would be calculated correctly. PTAL @xymostech! |
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.
Sorry for the long delay, @lencioni! This all looks pretty good to me! It's exciting to see it shrink the bundle! I'm mostly requesting changes because I don't know how rollup works and would like some clarification.
rollup.config.js
Outdated
input: `src/${filename}.js`, | ||
external: [ | ||
...Object.keys(pkg.dependencies), | ||
'inline-style-prefixer/static/createPrefixer', |
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.
Is there a way to configure this so we don't need to include all of these files individually?
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'm not a rollup expert, but I wasn't able to find a way initially. I'll dig into this more.
rollup.config.js
Outdated
], | ||
output: [ | ||
{ file: `lib/${filename}.js`, format: 'cjs' }, | ||
{ file: `lib/${filename}.mjs`, format: 'es' }, |
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.
What does this do?
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.
This will build a version that does not compile import
to require
, which enables build tools that can understand import
(e.g. webpack 2+) to perform additional optimizations such as "tree-shaking" and scope hoisting.
~/aphrodite ❯❯❯ head lib/index.js
'use strict';
Object.defineProperty(exports, '__esModule', { value: true });
function _interopDefault (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }
var createPrefixer = _interopDefault(require('inline-style-prefixer/static/createPrefixer'));
var calc = _interopDefault(require('inline-style-prefixer/static/plugins/calc'));
var crossFade = _interopDefault(require('inline-style-prefixer/static/plugins/crossFade'));
var cursor = _interopDefault(require('inline-style-prefixer/static/plugins/cursor'));
~/aphrodite ❯❯❯ head lib/index.mjs
import createPrefixer from 'inline-style-prefixer/static/createPrefixer';
import calc from 'inline-style-prefixer/static/plugins/calc';
import crossFade from 'inline-style-prefixer/static/plugins/crossFade';
import cursor from 'inline-style-prefixer/static/plugins/cursor';
import filter from 'inline-style-prefixer/static/plugins/filter';
import flex from 'inline-style-prefixer/static/plugins/flex';
import flexboxIE from 'inline-style-prefixer/static/plugins/flexboxIE';
import flexboxOld from 'inline-style-prefixer/static/plugins/flexboxOld';
import gradient from 'inline-style-prefixer/static/plugins/gradient';
import imageSet from 'inline-style-prefixer/static/plugins/imageSet';
The .mjs extension is the proposed way to specify this type of build that Node will eventually adopt, but it is really early to see it in many places. I'd be just as happy to build this somewhere like es/${filename}.js
if you prefer, and that might be a bit smoother for folks anyway until the dust settles on the .mjs extension.
src/no-important.js
Outdated
@@ -2,11 +2,26 @@ | |||
// Module with the same interface as the core aphrodite module, | |||
// except that styles injected do not automatically have !important | |||
// appended to them. | |||
import {defaultSelectorHandlers} from './generate'; | |||
import { defaultSelectorHandlers } from './generate'; |
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.
no-spaces in here is KA style
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.
Ah, my bad.
@@ -0,0 +1,22 @@ | |||
{ |
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.
What babel configuration were we using before? Can we try to stay consistent with that?
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.
Before this PR, Aphrodite was using a really old version of Babel, which was before presets existed and I believe Babel had a bunch of things turned on by default. When Babel 6 came out 2 years ago, everything changed and requires a lot more manual configuration.
I think the configuration I have here is going to be pretty consistent with what we had before, but I also made an effort to copy the browser targets we use for the CSS that is generated, which makes more sense to me than trying to stay consistent with the previous version of aphrodite. That way the JS and CSS is built to target the same sets of devices.
"env": { | ||
"test": { | ||
"presets": ["airbnb"], | ||
"plugins": ["istanbul"], |
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.
What does this do?
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.
This allows the code coverage to be properly calculated on the pre-compiled code. Without this, the code coverage checks failed. https://github.com/istanbuljs/babel-plugin-istanbul
@@ -2,95 +2,95 @@ import {assert} from 'chai'; | |||
|
|||
import OrderedElements from '../src/ordered-elements'; | |||
|
|||
describe("OrderedElements", () => { | |||
it("can identify elements it has", () => { | |||
describe('OrderedElements', () => { |
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.
not sure why all these quotes changed
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.
Just making things consistent. I can change this back if you like.
distBuild({ filename: 'aphrodite.umd.js', format: 'umd', sourcemap: true, minify: false }), | ||
distBuild({ filename: 'aphrodite.umd.min.js', format: 'umd', sourcemap: true, minify: true }), | ||
distBuild({ filename: 'aphrodite.js', format: 'cjs', sourcemap: false, minify: false }), | ||
standardBuilds('index'), |
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.
just to check my understanding, is this going to build bundles into lib/
or leave things in individual files like they are currently?
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.
This will build bundles into lib/
without the individual files. Here's what my lib/
directory looks like after building like this:
total 400
drwxr-xr-x 7 joe_lencioni staff 224B Oct 26 09:58 .
drwxr-xr-x 31 joe_lencioni staff 992B Oct 13 10:54 ..
-rw-r--r-- 1 joe_lencioni staff 46K Oct 26 09:58 index.js
-rw-r--r-- 1 joe_lencioni staff 45K Oct 26 09:58 index.mjs
-rw-r--r-- 1 joe_lencioni staff 46K Oct 26 09:58 no-important.js
-rw-r--r-- 1 joe_lencioni staff 45K Oct 26 09:58 no-important.mjs
-rw-r--r-- 1 joe_lencioni staff 4.4K Oct 26 09:57 staticPrefixData.js
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've updated this PR to build the cjs version into lib/ and the esm version into es/
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.
Thanks for the review! I'll wait to make any stylistic changes on this PR until I hear back from you that the more fundamental direction is okay.
@@ -0,0 +1,22 @@ | |||
{ |
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.
Before this PR, Aphrodite was using a really old version of Babel, which was before presets existed and I believe Babel had a bunch of things turned on by default. When Babel 6 came out 2 years ago, everything changed and requires a lot more manual configuration.
I think the configuration I have here is going to be pretty consistent with what we had before, but I also made an effort to copy the browser targets we use for the CSS that is generated, which makes more sense to me than trying to stay consistent with the previous version of aphrodite. That way the JS and CSS is built to target the same sets of devices.
"env": { | ||
"test": { | ||
"presets": ["airbnb"], | ||
"plugins": ["istanbul"], |
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.
This allows the code coverage to be properly calculated on the pre-compiled code. Without this, the code coverage checks failed. https://github.com/istanbuljs/babel-plugin-istanbul
rollup.config.js
Outdated
input: `src/${filename}.js`, | ||
external: [ | ||
...Object.keys(pkg.dependencies), | ||
'inline-style-prefixer/static/createPrefixer', |
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'm not a rollup expert, but I wasn't able to find a way initially. I'll dig into this more.
rollup.config.js
Outdated
], | ||
output: [ | ||
{ file: `lib/${filename}.js`, format: 'cjs' }, | ||
{ file: `lib/${filename}.mjs`, format: 'es' }, |
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.
This will build a version that does not compile import
to require
, which enables build tools that can understand import
(e.g. webpack 2+) to perform additional optimizations such as "tree-shaking" and scope hoisting.
~/aphrodite ❯❯❯ head lib/index.js
'use strict';
Object.defineProperty(exports, '__esModule', { value: true });
function _interopDefault (ex) { return (ex && (typeof ex === 'object') && 'default' in ex) ? ex['default'] : ex; }
var createPrefixer = _interopDefault(require('inline-style-prefixer/static/createPrefixer'));
var calc = _interopDefault(require('inline-style-prefixer/static/plugins/calc'));
var crossFade = _interopDefault(require('inline-style-prefixer/static/plugins/crossFade'));
var cursor = _interopDefault(require('inline-style-prefixer/static/plugins/cursor'));
~/aphrodite ❯❯❯ head lib/index.mjs
import createPrefixer from 'inline-style-prefixer/static/createPrefixer';
import calc from 'inline-style-prefixer/static/plugins/calc';
import crossFade from 'inline-style-prefixer/static/plugins/crossFade';
import cursor from 'inline-style-prefixer/static/plugins/cursor';
import filter from 'inline-style-prefixer/static/plugins/filter';
import flex from 'inline-style-prefixer/static/plugins/flex';
import flexboxIE from 'inline-style-prefixer/static/plugins/flexboxIE';
import flexboxOld from 'inline-style-prefixer/static/plugins/flexboxOld';
import gradient from 'inline-style-prefixer/static/plugins/gradient';
import imageSet from 'inline-style-prefixer/static/plugins/imageSet';
The .mjs extension is the proposed way to specify this type of build that Node will eventually adopt, but it is really early to see it in many places. I'd be just as happy to build this somewhere like es/${filename}.js
if you prefer, and that might be a bit smoother for folks anyway until the dust settles on the .mjs extension.
distBuild({ filename: 'aphrodite.umd.js', format: 'umd', sourcemap: true, minify: false }), | ||
distBuild({ filename: 'aphrodite.umd.min.js', format: 'umd', sourcemap: true, minify: true }), | ||
distBuild({ filename: 'aphrodite.js', format: 'cjs', sourcemap: false, minify: false }), | ||
standardBuilds('index'), |
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.
This will build bundles into lib/
without the individual files. Here's what my lib/
directory looks like after building like this:
total 400
drwxr-xr-x 7 joe_lencioni staff 224B Oct 26 09:58 .
drwxr-xr-x 31 joe_lencioni staff 992B Oct 13 10:54 ..
-rw-r--r-- 1 joe_lencioni staff 46K Oct 26 09:58 index.js
-rw-r--r-- 1 joe_lencioni staff 45K Oct 26 09:58 index.mjs
-rw-r--r-- 1 joe_lencioni staff 46K Oct 26 09:58 no-important.js
-rw-r--r-- 1 joe_lencioni staff 45K Oct 26 09:58 no-important.mjs
-rw-r--r-- 1 joe_lencioni staff 4.4K Oct 26 09:57 staticPrefixData.js
src/no-important.js
Outdated
@@ -2,11 +2,26 @@ | |||
// Module with the same interface as the core aphrodite module, | |||
// except that styles injected do not automatically have !important | |||
// appended to them. | |||
import {defaultSelectorHandlers} from './generate'; | |||
import { defaultSelectorHandlers } from './generate'; |
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.
Ah, my bad.
@@ -2,95 +2,95 @@ import {assert} from 'chai'; | |||
|
|||
import OrderedElements from '../src/ordered-elements'; | |||
|
|||
describe("OrderedElements", () => { | |||
it("can identify elements it has", () => { | |||
describe('OrderedElements', () => { |
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.
Just making things consistent. I can change this back if you like.
@xymostech can you take another look at this when you have a moment? |
@xymostech since we decided that #240 is a breaking change, I'd like to ship this at the same time. Can you look at this again and let me know if you see anything you'd like changed about it? |
Rollup will produce a smaller and more optimized bundle than webpack, and can be configured in a way that works perfectly for libraries, such as Aphrodite. This will help to minimize the bundle size impact of using this package, and may even give a small runtime speedup. For reference, React 16 is built using Rollup. Rollup does not allow Babel 5, so I also updated to Babel 6 at the same time. In this update, I tried to take care to maintain the same list of browser support that we have listed in the CSS prefixes that we build. At some point, we probably want to unify this configuration via browserslist. Issue: #239 I noticed that this Babel update caused branch coverage to drop a little, and I was unable to fix it by adding a test that definitely covered the missing branch. Thankfully, all I needed to do was add the istanbul Babel plugin to fix this. This is the approach recommended by the istanbul documentation: https://github.com/istanbuljs/nyc#use-with-babel-plugin-istanbul-for-babel-support Along with this update, I decided to add an ES modules build since it was easy enough. This will be used automatically by tools such as webpack 2+ to import the ES6 module version directly. I think it still makes sense to run these through Babel since most people don't run their node_modules through Babel, so the main difference here is that the import/export statements will not be compiled to require/module.exports. This allows webpack to perform optimizations such as tree-shaking and scope hoisting. One risk to be on the lookout for when people update to this version is that if you are using `require` to bring in Aphrodite with a version of webpack that is ES modules capable, it will break. Those consumers will need to switch to import instead. For this reason, I would be okay with removing the `module` field from the package.json for an initial release of the rollup build, and then we can add it later when the ecosystem has time to catch up. This is the approach we landed on for react-waypoint: 1. civiccc/react-waypoint#220 2. civiccc/react-waypoint#223 The filesize of the dist builds before this change looked like: ``` 83K aphrodite.js 84K aphrodite.umd.js 104K aphrodite.umd.js.map 23K aphrodite.umd.min.js 204K aphrodite.umd.min.js.map ``` And after this change: ``` 72K aphrodite.js 73K aphrodite.umd.js 108K aphrodite.umd.js.map 20K aphrodite.umd.min.js 93K aphrodite.umd.min.js.map ``` So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
Going to merge now to keep things moving forward. Please let me know if you'd like me to follow up with any changes! |
Rollup will produce a smaller and more optimized bundle than webpack,
and can be configured in a way that works perfectly for libraries, such
as Aphrodite. This will help to minimize the bundle size impact of using
this package, and may even give a small runtime speedup. For reference,
React 16 is built using Rollup.
Rollup does not allow Babel 5, so I also updated to Babel 6 at the same
time. In this update, I tried to take care to maintain the same list of
browser support that we have listed in the CSS prefixes that we build.
At some point, we probably want to unify this configuration via
browserslist. Issue:
#239
Along with this update, I decided to add an ES modules build since it
was easy enough. This will be used automatically by tools such as
webpack 2+ to import the ES6 module version directly. I think it still
makes sense to run these through Babel since most people don't run their
node_modules through Babel, so the main difference here is that the
import/export statements will not be compiled to require/module.exports.
This allows webpack to perform optimizations such as tree-shaking and
scope hoisting.
One risk to be on the lookout for when people update to this version is
that if you are using
require
to bring in Aphrodite with a version ofwebpack that is ES modules capable, it will break. Those consumers will
need to switch to import instead. For this reason, I would be okay with
removing the
module
field from the package.json for an initial releaseof the rollup build, and then we can add it later when the ecosystem has
time to catch up. This is the approach we landed on for react-waypoint:
The filesize of the dist builds before this change looked like:
And after this change:
So it looks like the minified UMD build dropped from 24 KiB to 20 KiB.
cc @vladnicula