Single-file bundling is still preventing useful tree-shaking

[billyjanitsch]

Version

react-router-dom@4.4.0-beta.7

Test Case

https://github.com/billyjanitsch/react-router-tree-shaking

Steps to reproduce

Clone the repo above, run npm run build, then observe the output in dist/main.js.

Expected/Actual Behavior

Continuation of #6464.

@mjackson thanks for trying to resolve this. Unfortunately, the same issue as originally described for 4.4.0-beta.6 persists in 4.4.0-beta.7.

I've updated the example repo to show this. The repro instructions are the same as before, and as before, all three history types are included in the bundle even though only BrowserRouter is imported.

I still think the proper fix is to stop bundling the libraries into single JS files, outlined in #6464 (comment).

To summarize, {sideEffects: false} allows Webpack to prune unused files (rather than relying on a minifier to prune unused code paths, which doesn't work very well). But it can only prune entire files, so it only works when code is split between multiple files. That's why {sideEffects: false} is not useful if the library is bundled into a single file.

[mjackson]

Thanks for checking, @billyjanitsch.

I still think the proper fix is to stop bundling the libraries into single JS files

It's more than a little frustrating that the limitations of webpack dictate how we structure our modules. My strategy up to this point has been to rely on webpack's minifier (uglify) to eliminate dead code, but, as you say, it's not working very well.

I did quite a bit of research into this a few months ago, where I determined that we should be able to build everything into a single file while relying on uglify to strip out all the code we weren't using. It worked fine there (on the 2nd attempt). Not sure why it's not working very well now. I need to dig in again.

Quick question: using your repo, how are you determining that tree-shaking isn't working? When I ran npm run build in the console it told me what the bundle size was, but I didn't get very descriptive output. This has been the most difficult part of this task for me: figuring out an easy way to tell exactly which modules webpack is including and which code it's able to remove.

[StringEpsilon]

@billyjanitsch Are you sure the treeshaking isn't working?

Using your repository, I checked the bundle composition with webpack-bundle-analyzer and I see the following stats:

module	size
history/esm	8,35 kB
react	6,59 kB
path-to-regexp	3.07 kB
react-router/esm	2.82 kB
create-react-context/lib	2.75 kB
react-is	2.16 kB
react-router-dom/esm	494 B

• a few more dependencies less than 1kB

According to bundlephobia, the (self-)size of react-router-dom is 2.8kB (~2.4 kB more) and the size of react-router is 6.36kB (~4kB more).

Edit: Looks to me like only the history bundle isn't properly tree-shaken. But that should probably be reported on the history repository.

[billyjanitsch]

Quick question: using your repo, how are you determining that tree-shaking isn't working? When I ran npm run build in the console it told me what the bundle size was, but I didn't get very descriptive output. This has been the most difficult part of this task for me: figuring out an easy way to tell exactly which modules webpack is including and which code it's able to remove.

@mjackson I look directly at the output file (dist/main.js), but I realize this is a pain.

I made an unmangle branch of my repo that should help you explore. 🙂 I configured the minifier to avoid mangling variable names and set up Prettier to run on the output. I also committed the output file for reference. (Note that the bundle size info is no longer accurate since avoiding mangling obviously results in a larger bundle, but the output is accurate as far as which code gets eliminated.) Here's the example I gave of code that should be tree-shaken but isn't (createHashHistory, as explained in #6464).

Looks to me like only the history bundle isn't properly tree-shaken. But that should probably be reported on the history repository.

@StringEpsilon as you point out, this code is coming from the history package. But, no, this shouldn't be reported there, because tree-shaking works better if you import directly from that package. See this branch of my repo, where I've updated the entry file to import {createBrowserHistory} from 'history' instead of importing anything from react-router. In this case, you can see that createHashHistory is successfully tree-shaken from the output, so history itself is not the problem (although it may also benefit further from being split into multiple files).

The problem is that when react-router is bundled into a single file, all of the history imports are consolidated in that file. This forces webpack to mark all of the associated history exports as used (see #6464 (comment)) because it has no way of knowing at the time that some of those uses will be eliminated later by the minifier. Instead, if react-router consisted of several modules which each imported the needed bits of history, then if/when webpack prunes one of those modules, its associated history exports would never be marked as used. For example, pruning react-router's HashHistory module would let webpack mark the createHashHistory history export as unused, allowing it to be eliminated (either via further module pruning if history also consisted of multiple files, or at least via dead code elimination if not).

I hope that makes sense -- it's hard to explain but I tried my best. 😅

It's more than a little frustrating that the limitations of webpack dictate how we structure our modules.

@mjackson I understand your point of view, but the way I think about it is that tree-shaking (with {sideEffects: false}) is an opt-in optimization feature provided by webpack that you can choose to use if you want. But it fundamentally works by statically analyzing the module import/export graph as opposed to the module contents, so if you don't give it a "graph" (just a single file), there's not much it can do.

FWIW, even if it worked as intended, I feel like modifying source code to make Uglify happier (e.g. #6465, which is detrimental because defaultProps are useful in the React dev tools) is an uglier workaround than not rolling up the library. But I realize the subjectivity of that preference. 🙂

In any case, thanks again for looking into this!

[mjackson]

Thanks for following up here @billyjanitsch.

At this point we have done as much as we can to enable tree-shaking w/out making special concessions for webpack (which doesn't really do tree-shaking). All of react-router, react-router-dom and history packages are tree-shakable using a tool that actually does tree-shaking (i.e. Rollup), which means that this isn't really a problem with our code. This is a problem with webpack.

Having said that, here are a few points to consider:

• React itself has this same problem. React currently uses single-file builds, which will have this exact same problem if there are parts of the library that you're not using (e.g. React.Component is not needed in some apps that don't ever use it).
• You can work around this in React Router by avoiding the places where we use history as a transitive dependency. For example, you could do something like:

// Import directly from history
import { createBrowserHistory } from 'history';
// Import Router directly instead of BrowserRouter
import { Router } from 'react-router-dom';

const history = createBrowserHistory();

<Router history={history}>
  // ...
</Router>

I actually forked your repo and experimented with this approach in the separate-imports branch to prove that it works.

If you don't like that workaround, you can always switch to using Rollup for bundling your app. Granted, the Rollup ecosystem isn't quite as developed as webpack currently is, but it's getting there.

All of this is not to say we don't care about your bundle size; we absolutely do and we are actively working to get our bundle size down. Our initial estimates of the bundled size of version 5 are around 3kb right now. That's smaller than our path-to-regexp dependency all by itself 😅

But for now, I need to ship 4.4 and move on. This issue has been delaying the release for a few weeks now.

Hope that helps!

[billyjanitsch]

@mjackson once again, thanks so much for the time you've spent looking into this. ☺️

But for now, I need to ship 4.4 and move on. This issue has been delaying the release for a few weeks now.

I totally agree that there's no reason for this to block 4.4, and I apologize if I came across as suggesting that 4.4 shouldn't be released until this was resolved. Congratulations on shipping. 🙂

That said, now that 4.4 is out, would you consider re-opening this to continue discussion for future versions?

All of react-router, react-router-dom and history packages are tree-shakable using a tool that actually does tree-shaking (i.e. Rollup), which means that this isn't really a problem with our code. This is a problem with webpack.

If you don't like that workaround, you can always switch to using Rollup for bundling your app.

Unfortunately, this isn't the case. See mjackson/react-router-tree-shaking#1, where I've replaced Webpack with Rollup and still found that tree-shaking doesn't work. This doesn't seem to be an issue of Rollup vs. Webpack, but rather a limitation in static module analysis + DCE when packages are pre-bundled into single files.

You can work around this in React Router by avoiding the places where we use history as a transitive dependency.

Thanks for suggesting. It's a good idea, but this workaround prevents Link and NavLink from ever being used (see mjackson/react-router-tree-shaking#2 for details). Would you agree that this is a pretty big pitfall for typical apps?

React itself has this same problem. React currently uses single-file builds

(⚠️ Below is a bit of a tangent, just responding to your point.)

That's true, but I'm not sure it's a fair comparison. Tree-shaking mostly benefits libraries which expose multiple, somewhat independent pieces of similar size which aren't all likely to be used, and React is not such a library. Also, the advantages that React buys itself by pre-bundling are larger than those received by libraries which only use Rollup to put everything in one file.

To elaborate, the react package is small (4x smaller than react-router-dom) and most of it is used in a typical app, so there's comparatively little to gain from tree-shaking. Most of the size is in react-dom, in the form of a single, indivisible object (the renderer), so tree-shaking wouldn't provide value there either. That's why it hasn't been a priority for the React team.

There's also the fact that React uses advanced compile-time tooling (e.g. GCC) to produce a maximally small/performant build, using optimizations that would be very slow or impossible to reproduce in users' bundling setups. This necessitates shipping a single file. This is different from plain Rollup, which, when bundling libraries, roughly just concatenates the modules in the right order. i.e. React gets something from doing this that other libraries don't.