Build with rollup #220

lencioni · 2017-10-02T21:06:57Z

Previously, we were building waypoint just by running it through Babel.
Since we split things out into separate modules, this caused a bit of
extra overhead in the bundle size and at runtime due to the extra
wrapping of these modules.

Thankfully, rollup was built to help us produce a smaller build. To make
this work, I needed to remove the Babel configuration that specifies a
module transformer, so I decided to switch to the Airbnb Babel preset,
which exposes an option for this (and under the hood uses
babel-preset-env, which we need to migrate to anyway). More info:

https://github.com/rollup/rollup-plugin-babel#configuring-babel

The rest of this was loosely based off the rollup-starter-lib
configuration. This gives us an ESM build pretty much for free.

https://github.com/rollup/rollup-starter-lib/tree/babel

Previously, we were building waypoint just by running it through Babel. Since we split things out into separate modules, this caused a bit of extra overhead in the bundle size and at runtime due to the extra wrapping of these modules. Thankfully, rollup was built to help us produce a smaller build. To make this work, I needed to remove the Babel configuration that specifies a module transformer, so I decided to switch to the Airbnb Babel preset, which exposes an option for this (and under the hood uses babel-preset-env, which we need to migrate to anyway). More info: https://github.com/rollup/rollup-plugin-babel#configuring-babel The rest of this was loosely based off the rollup-starter-lib configuration. This gives us an ESM build pretty much for free. https://github.com/rollup/rollup-starter-lib/tree/babel

lencioni · 2017-10-02T21:07:22Z

I haven't used rollup before, so it is likely I missed something important. Please keep this in mind while reviewing.

ljharb

rollup isn't spec-compatible with import * as, so I'd both caution against using it, and also be very careful about using star imports.

ljharb · 2017-10-02T21:46:28Z

package.json

@@ -2,7 +2,8 @@
  "name": "react-waypoint",
  "version": "7.2.0",
  "description": "A React component to execute a function whenever you scroll to an element.",
-  "main": "build/waypoint.js",
+  "main": "build/index.cjs.js",
+  "module": "build/index.esm.js",


If you want to be compatible with what node will support, you'll want to do build/index.js and build/index.mjs here.

@ljharb

As suggested by @ljharb in code review, this will be compatible with what node will support.

jamesplease · 2017-10-02T22:30:37Z

Good idea here, @lencioni !

I'll give a more thorough review tonight, but I thought I'd pop in here to suggest that we stick with .js for now, even for the module field in package.json..mjs support in Node is experimental, so I think we should wait until the spec is finalized, and the flag is removed, before we include .mjs files in this project. Just my two cents ✌️

jamesplease · 2017-10-02T22:45:40Z

Overall, this looks good. Two questions:

Do we want to export in the UMD format for browser-users? We could do that by adding format: 'umd', to the Rollup config, as well as using the globals option, which is a map of globals variables to the library names that we import.

Also, for the ES modules build, what do we get out of running it through Rollup? Could we just run the lib through Babel? This is the approach that Redux takes, and I've had success on it on personal projects, too.

ljharb · 2017-10-02T22:56:35Z

Regardless of the module field, .mjs in node is a foregone conclusion - if you want to omit that field, fine, but an .mjs file is the proper way moving forward for the JS ecosystem, full stop.

jamesplease · 2017-10-02T23:19:07Z

Sure, but it is still currently an experimental feature. It'll likely work out fine either way, but we should just be aware of the fact that we're adding in a thing that depends on a feature that is, at this time, experimental. If we're cool with that, then 👍

ljharb · 2017-10-02T23:50:17Z

Fair point. In that case, there shouldn't be a "module" field at all because that's not a feature that's standardized in any way whatsoever :-)

jamesplease · 2017-10-03T00:13:24Z

In that case, there shouldn't be a "module" field at all because that's not a feature that's standardized in any way whatsoever :-)

I agree with you that we may not need the module field, but not because it's not standardized. You may already know this, but common build tools recognize this field, and will prefer it over main.

This can be useful for tree shaking, but given this library's single export, I don't believe that there is anything to shake. For this reason, we could probably just get away with just exporting a single UMD build as main.

ljharb · 2017-10-03T00:37:47Z

I'm not sure why a UMD build would even be needed - just a single CJS file could work too.

jamesplease · 2017-10-03T00:42:15Z

I'm not sure why a UMD build would even be needed - just a single CJS file could work too.

You're probably right. I don't have any numbers on this, but I suspect that the number of React developers who aren't using some kind of build tool is miniscule. Maybe bundling UMD is a holdover from some previous time, but I still find myself doing it. It's a simple thing for me to add to my libraries, and it could help a developer trying to consume the library.

I'd be fine with us just shipping a CJS file as main, though.

trotzig · 2017-10-03T13:06:18Z

It looks like build failures are spurious. I've manually retriggered the failing ones.

lencioni · 2017-10-03T16:05:51Z

Also, for the ES modules build, what do we get out of running it through Rollup? Could we just run the lib through Babel?

I think we get two things. The first is that it cuts down on the amount of work that app bundlers will need to do at build time. The second is that it makes all of the rolled up and not exported functions private to the module, which gives us more confidence when deciding what is a breaking change or not.

In that case, there shouldn't be a "module" field at all

I'd like to keep the module field because at Airbnb, we are working on improving our builds in ways that will be able to take advantage of this. I don't care if the file is called index.mjs or spaghetti.and.meatballs. We can change it later if we need to without much hassle. I think index.mjs is fine. Are you comfortable with this @jmeas? If not I'm happy to change it back to index.esm.js.

This can be useful for tree shaking, but given this library's single export, I don't believe that there is anything to shake. For this reason, we could probably just get away with just exporting a single UMD build as main.

I think it can also be useful for scope hoisting. It's true that we only have one export, but we do import a few other things that may benefit from some of these optimizations. And I believe that those imports need to be using import and not require (which is the only real difference between the main and module versions that rollup produces here).

If any tree shaking could happen in this package, I think rollup would be shaking it out here.

I'm not sure why a UMD build would even be needed - just a single CJS file could work too.

Agreed, I don't think we need UMD. It's not something we have now and nobody has asked for it. We can always add it later, but that is tangential to what I am hoping to accomplish here.

jamesplease · 2017-10-03T16:39:26Z

Ah, I should have checked – I just assumed we were currently already bundling a UMD build. I agree that it’s outside of the scope of this PR to add that in.

I also agree with the other points you mentioned. As long as we’re absolutely certain that we don’t want to go with spaghetti.and.meatballs, let’s get this thing merged!

ljharb

woot

ljharb · 2017-10-03T16:42:50Z

package.json

@@ -2,7 +2,8 @@
  "name": "react-waypoint",
  "version": "7.2.0",
  "description": "A React component to execute a function whenever you scroll to an element.",
-  "main": "build/waypoint.js",
+  "main": "build/index.js",


if both of these are just "build" - which is the best practice - does that mean rollup can't figure out the destination filenames?

I don't understand your question. What is a best practice? What is rollup not figuring out?

"main": "build", "module": "build".

I don't see anywhere else index.js or index.mjs are specified tho, which leads me to believe that rollup infers the filenames from the package.json fields.

No, it doesn't infer anything. It is configured with an explicit dependency on these fields in the rollup config. https://github.com/brigade/react-waypoint/pull/220/files#diff-ff6e5f22a9c7e66987b19c0199636480

+ targets: [ + { dest: pkg.main, format: 'cjs' }, + { dest: pkg.module, format: 'es' } + ],

That specifies the format, but not the file extension. Where would spaghetti and meatballs go?

It specifies the path currently. If we change it to spaghetti and meatballs, it would go in the package.json.

Right - that confirms my initial hunch, which is that putting just "build" wouldn't work, because it would deprive rollup of the actual path.

jamesplease · 2017-10-03T18:09:20Z

rollup.config.js

+      ...Object.keys(pkg.peerDependencies),
+    ],
+    targets: [
+      { dest: pkg.main, format: 'cjs' },


I just looked this up after reading @ljharb 's questions, and I don't see targets in the Rollup docs. Should this instead be output and file, rather than targets and dest?

Looks the answer might be yes

Ooh, in that case yes that's better :-D

Ah, good catch. I added a commit to convert these.

I looked into updating the code I pulled this from and it looks like there is an open PR for it already! rollup/rollup-starter-lib#2

These options were renamed recently. Updating to match the documentation. More information: https://gist.github.com/Rich-Harris/d472c50732dab03efeb37472b08a3f32

jamesplease · 2017-10-04T00:29:20Z

🎉

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

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

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.

lencioni requested review from trotzig and jamesplease October 2, 2017 21:06

ljharb suggested changes Oct 2, 2017

View reviewed changes

Use index.js and index.mjs for built artifacts

bf06b2b

As suggested by @ljharb in code review, this will be compatible with what node will support.

jamesplease approved these changes Oct 3, 2017

View reviewed changes

ljharb approved these changes Oct 3, 2017

View reviewed changes

jamesplease reviewed Oct 3, 2017

View reviewed changes

Update rollup config for rollup 0.48

f4296e7

These options were renamed recently. Updating to match the documentation. More information: https://gist.github.com/Rich-Harris/d472c50732dab03efeb37472b08a3f32

jamesplease approved these changes Oct 3, 2017

View reviewed changes

lencioni merged commit 8a89c4d into master Oct 3, 2017

lencioni deleted the rollup branch October 3, 2017 23:52

trotzig mentioned this pull request Oct 4, 2017

Error after updated to v7.3.0 #221

Closed

lencioni mentioned this pull request Oct 12, 2017

Build with rollup instead of webpack Khan/aphrodite#281

Merged

trotzig mentioned this pull request Nov 17, 2020

Use .mjs for module build #339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build with rollup #220

Build with rollup #220

lencioni commented Oct 2, 2017

lencioni commented Oct 2, 2017

ljharb left a comment

ljharb Oct 2, 2017

jamesplease commented Oct 2, 2017

jamesplease commented Oct 2, 2017 •

edited

Loading

ljharb commented Oct 2, 2017

jamesplease commented Oct 2, 2017 •

edited

Loading

ljharb commented Oct 2, 2017

jamesplease commented Oct 3, 2017

ljharb commented Oct 3, 2017

jamesplease commented Oct 3, 2017

trotzig commented Oct 3, 2017

lencioni commented Oct 3, 2017

jamesplease commented Oct 3, 2017

ljharb left a comment

ljharb Oct 3, 2017

lencioni Oct 3, 2017

ljharb Oct 3, 2017

lencioni Oct 3, 2017

ljharb Oct 3, 2017

lencioni Oct 3, 2017

ljharb Oct 3, 2017

jamesplease Oct 3, 2017

jamesplease Oct 3, 2017

ljharb Oct 3, 2017

lencioni Oct 3, 2017

lencioni Oct 3, 2017

jamesplease commented Oct 4, 2017

Build with rollup #220

Build with rollup #220

Conversation

lencioni commented Oct 2, 2017

lencioni commented Oct 2, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease commented Oct 2, 2017

jamesplease commented Oct 2, 2017 • edited Loading

ljharb commented Oct 2, 2017

jamesplease commented Oct 2, 2017 • edited Loading

ljharb commented Oct 2, 2017

jamesplease commented Oct 3, 2017

ljharb commented Oct 3, 2017

jamesplease commented Oct 3, 2017

trotzig commented Oct 3, 2017

lencioni commented Oct 3, 2017

jamesplease commented Oct 3, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease commented Oct 4, 2017

jamesplease commented Oct 2, 2017 •

edited

Loading

jamesplease commented Oct 2, 2017 •

edited

Loading