Switch build to Rollup?

[pshrmn]

I just wanted to bring this up since there is some discussion happening related to the current builds (#5095).

I've been using Rollup in some of my projects to output a single file for each format and enjoying it. For React Router, that would mean that instead of a dozen commonjs files, a dozen es files, and two UMD files, we would end up with four total files (react-router.common.js, react-router.es.js, react-router.js, and react-router.min.js) . I think that that is the direction Facebook is heading with React and I know that that is what Vue is already doing.

The only real "concern" I've had with that is source maps. Rollup can generate them for you, that isn't the problem. It is just that I end up having to use the source-map-loader to prevent the browser for asking for them. It is just a minor annoyance, but something to take into consideration. Vue doesn't even ship with sourcemap files, although I'm not sure what the reasoning behind that is.

Another thing to think about is that only the top level exports get exported. Whether or not this is a good thing probably depends on who you ask. I like it because you don't have to worry about people importing private APIs from the package. If it isn't exported from index.js, they can't import it. I suppose that technically that is a breaking change for people who do import Route from 'react-router/Route'. It doesn't break the API, though, so I think that a minor bump would be enough.

If there is interest, I can put together a PR to do this. I might go ahead and actually do that anyways to get an idea of the output file sizes.

[timdorr]

Of note, we switched over Redux and it's been great. Cleaner and smaller builds are great.

Also, react-router-redux is already using it.

[pshrmn]

It looks like Redux only uses it for the UMD builds, though, right? Are there plans for the main/module builds to be done with Rollup?

[timdorr]

Yes

[pshrmn]

File sizes aren't the only reason to do this, but since I mentioned them above, here are my results.

Current:

Filename	Size	Gzip Size
react-router.js	75.13	15.1
react-router.min.js	27.33	7.14
react-router-dom.js	135.73	23.82
react-router-dom.min.js	51.8	11.05

Rollup:

Filename	Size	Gzip Size
react-router.js	78.86	19.13
react-router.minjs	20.32	6.54
react-router-dom.js	149.7	33.02
react-router-dom.min.js	40	9.91

The full size files are actually slightly bigger, but the minimized files are all smaller. 🤷‍♀️ I believe that I have identified the cause, which is the prop-types module. Currently, the full sized build sets the NODE_ENV to production, resulting in prop-types being shimmed. However, I think that that is an error because I don't believe that the non-minimized UMD is supposed to be in production. If I build with that in development, the current files are larger that the rollup ones.

[mjackson]

Thanks for bringing this up. It's been on my mind for some time now, but I just keep using webpack because it's the thing I already know.

We currently use webpack in 3 places:

• UMD builds
• test bundles
• the website

I realize you're only suggesting we switch the UMD builds to use Rollup, but I'd like to avoid a situation where we're using 2 different bundlers in the same repo (I didn't realize react-router-redux was already using Rollup). We were doing that for a while with Browserify and webpack, and I had a hard time keeping it straight which bundler + config was being used for what.

We can pretty easily switch the UMD builds to use Rollup.

I'm currently working on porting the test suite to Jest, so I think we'll be able to ditch the webpack
(via karma) usage in the tests.

We'll probably want to stick with webpack for the website, unless you're really determined to figure something else out.

So for now I'd probably say let's just stick with webpack everywhere. Does that make sense? Or am I just being lazy?

However, I think that that is an error because I don't believe that the non-minimized UMD is supposed to be in production.

Ya, I think you're right. We should probably change this line to say development instead of production.

[pshrmn]

I was actually suggesting all of the builds, not just UMD.

I have it working for react-router and react-router-dom. The other packages should be fairly trivial to add since it is mostly copy+pasting. I added a rollup branch, so you can pull and run that to see the output. There is also a diff here, but it is a bit messy right now. I swapped to jest because I didn't want to deal with karma if its being dropped, so a good bit of that diff is just removing karma related things. Once you update the master branch to jest, I can rebase and it would be easier to see the rollup changes.

I think that it would be fine to have the website built with webpack while everything else is rollup. That would keep to the "Use webpack for apps, and Rollup for libraries" idea.

[mjackson]

Sorry if I'm a little slow here, but what advantage is there to building our ES + CommonJS builds with rollup over just using babel? Also, I'm a bit confused as to why Rollup generates a single file instead of multiple files like we currently do for those builds. AFAICT we don't actually want a bundle for those builds. The separate files are what makes it easy for people to import only the specific files they need.

[pshrmn]

I was just reading through that Redux thread Tim linked earlier and saw one of his comments there:

Anecdotally, I would never apply this to a library like React Router, as one of the bundle-slimming techniques we use is encouraging direct imports (import Router from 'react-router/Router') as sort of a poor man's tree shaking. Bundling everything together, including unused components, would have a negative effect on bundle sizes in that case.

The loss of "poor man's tree shaking" would definitely upset some people. It is definitely an extreme change to switch to a single file we could just switch to Rollup for the UMD (at least for the time being). The way that I look at this is:

1. If someone is using import XX from 'react-router-dom/XX' to reduce output size, we can safely say that they are using some sort of bundler.
2. A single file would really only be affecting bundles. There is no bundle to worry about on the server, so there isn't a cost to be paid for switching to import { XX } from 'react-router-dom on the server.
3. Webpack 2, which added tree-shaking (although the actual code removal is only done when doing a production build with --optimize-minimize), was released in January 2017. React Router v4 was released in March 2017. Obviously not everyone uses Webpack (or Rollup) and of those that do, some will be stuck on v1. Still, while I have no numbers, for new projects that use Webpack, I would imagine the majority of them are going to be on Webpack 2+. This means that they do not need "poor man's tree shaking".

Maybe it is too early to switch to a single file, but I think that at some point in time it will make sense to say "rely on your bundler" for optimization instead of cherry picking files. Then, we can still stick to multiple files, but that is more of an aesthetic choice.

I'll revert the main/module builds on my rollup branch and maybe that can be reconsidered for v5.

[pshrmn]

With the tests switched to jest, the diff between master and rollup is now a lot easier to go through: master...rollup.

The only "controversial" thing might be that I moved the entire Babel config to a js file and just import that in the .babelrc. There was some configuration that I wanted to add and it made more sense to just centralized everything there. Babel 7 is going to support .babelrc.js files, so eventually that file could just be moved and renamed and the .babelrc could be removed.

[timdorr]

Since you've got it checked out and installed locally already, would you mind posting a gist with the output from Webpack and Rollup of the uncompressed UMD? Just to show the readability differences. For example, here are Redux's:

Rollup UMD build
Webpack UMD build

Much cleaner to read through, which is important for good DX.

I still maintain Webpack is for apps (like the website) and Rollup is for libs.

BTW, I'm not totally against building the ES and CJS as bundles too. Those are also really clean:

Babel index.js
Rollup ES bundle

It's definitely worth exploring here.

[pshrmn]

react-router.js webpack vs. rollup

react-router-dom.js webpack vs. rollup

The only thing that I don't love about Rollup is the way that the commonjs plugin creates variable names. When you have a lot of dependencies that only ship commonjs modules, you end up with a bunch of index variables.

// someModule/index.js
module.exports = { ... }

// is transformed to
var index = { ... }
export default index

Not a huge concern, but prettier names would be nice for reading. I opened up a PR there, so we'll see if that changes gets considered (rollup/rollup-plugin-commonjs#208).

[mjackson]

I think that at some point in time it will make sense to say "rely on your bundler" for optimization instead of cherry picking files

I really wish this were the case, and I thought we were already there with webpack 2 but it doesn't look like it. In that PR, we're actually reverting back to cherry-picking files because webpack doesn't know how to tree-shake export { A as default } from './a'. I definitely agree with you that at some point we will get there, but I think we're still a ways out from just being able to rely on our bundlers to magically do the right thing.

Would you be open to reducing the scope of this PR to use rollup for only the UMD builds? We can keep using babel for es/cjs so people can keep cherry-picking and perhaps take those up in a separate issue when it makes sense.

[pshrmn]

The current rollup branch only does UMD master...rollup

As far as that PR goes, I want to say that the problem would disappear if we were using a single file, but I'd have to do some testing to figure out if that is true. I'll try to put together a demo project sometime soon and see whether a single file release will only include the relevant files. I've rethought my earlier statement and think that it would be appropriate to switch to single file main/module builds in a major release, so I don't think that there is a rush in that regard.

[mjackson]

Ah, sorry. I just assumed it was doing what you said before. I'll take another look at the code.

[mjackson]

Well, this all looks 👍 to me. Ready to merge, @pshrmn?

[pshrmn]

I should probably switch react-router-config to rollup first.

[mjackson]

Are we concerned about externalizing the prop-types package as well? @jochenberger brought it up in #5045, but it seems like just another thing we would have to ask people to put on the page in order to use the router.

[pshrmn]

Ideally prop-types would be external, but I think that it should probably be bundled. Even if a user is stripping all of their prop types in production, we rely on them for context. Its a little bump to the file size, but a lot more convenient for developers. Even more convenient was when React provided them, but c'est la vie.

[pshrmn]

Alright, I added react-router-config support and opened #5343. The rollup.config.js for react-router-config is a little bit more involved because that package uses cherry picked imports. It would be simpler if it was just importing from react-router, but I didn't want to add that change to the scope of this PR.

[mjackson]

Closed by #5343

[pshrmn]

Just an update about the idea of moving the other builds to rollup: for the time being, we cannot ☹️

https://twitter.com/Rich_Harris/status/887993633823240192

Basically, every class C extends React.Component gets compiled to something like this:

var inherits = function(sub, super) {...}
var C = function(_ReactComponent) {
  inherits(C, _ReactComponent);
  function C () {...};
  return C;
}(React.Component);

The inherits function can't be analyzed to say that it has no effects, so even if C is not imported by a module, it cannot be tree shaken (doesn't work with either Webpack or Rollup). It's a shame because from what I can tell, that means that no class based React components can be tree shaken. I suppose that a project could revert to createClass... Not necessarily ideal, but not the worst idea.

I'm hoping that in the future we can inline the inherits function to get this to work. Babel 7 is introducing an "inherits loose" function. When I manually inline its body into the compiled class, tree shaking appears to work. (Rollup REPL example) Any changes would have to come through Babel, probably with a custom plugin (adapted from transform-es2015-classes).