Bundling packages into single files is preventing useful tree-shaking

[billyjanitsch]

Version

react-router-dom@4.4.0-beta.6

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 Behavior

The biggest gains to be had from tree-shaking in react-router/history are probably removing whichever of browser/hash/memory histories are not used by the app. For example, if only BrowserRouter is imported from react-router-dom, one would expect the hash and memory routers, along with their associated histories, to be tree-shaken.

Actual Behavior

Neither of the unused histories are tree-shaken. You can verify this by searching for hashType in the build output, which only appears in createHashHistory and therefore should have been tree-shaken from the bundle. As a result, the output bundle is quite large: importing only BrowserRouter yields a 33.2 kB bundle, whereas importing the entire library yields a barely-larger 37.9 kB bundle.

I believe this happens because webpack is generally not good at tree-shaking unused parts of a module -- it's much better at tree-shaking entire unused modules (particularly when sideEffects: false has been set). For example, if createHashHistory lived in a separate module, webpack would know that the module import was unused and prune the entire module.

What was the motivation for bundling history, react-router, and react-router-dom into single files on npm? I understand why it's more efficient to do this for CJS modules, but for ESM, it seems strictly better to leave the bundling to the bundler, since modern bundlers are capable of inlining ESM without adding glue code, with the advantage that they can be smarter about pruning entire modules.

(Another theory is that this re-export is the problem.)

/cc @TrySound @mjackson

[pshrmn]

Can you install history@next and check the build results? That release uses sideEffects: false, so I'm curious to see what effect it has on the build.

[billyjanitsch]

@pshrmn my test repo linked above already uses history@next (it's transitively depended on by next versions of react-router-dom and react-router)

(sideEffects: false only affects webpack's ability to prune entire module files, though, which is why I'm asking about history being bundled into a single file.)

[pshrmn]

Oh, I glanced at the wrong package.json 🤦‍♂️

[timdorr]

AFAIK, only Rollup does built-in DCE for flat bundles. Webpack outsources that to a minifier (UglifyJS by default). Is this the size including minification? I would imagine a large chunk of that will disappear once something actually eliminates the dead code.

[billyjanitsch]

@timdorr yes, DCE via minification is being applied. See the test repo, where webpack's mode is set to production, enabling the Uglify plugin. The sizes I listed are DCE/minified, which you can verify by looking at the collapsed NODE_ENV ternaries in the output bundle.

The outsourcing you describe is exactly the problem: Uglify is not so good at DCE, especially of inter-dependent functions defined in a single module. webpack generally gets much better results when it can prune entire modules based on static imports/exports, which is why the sideEffects flag was introduced.

[TrySound]

@billyjanitsch Yep, something blocks treeshaking here. Will take a look.
https://github.com/ReactTraining/react-router/blob/master/packages/react-router-dom/.size-snapshot.json#L12

[timdorr]

I wonder if Terser or babel-minify would be able to handle this better. I'm fairly certain babel-minify can do it. Not that it's a solution, mind you. It leaves those using the default config out in the cold :/

[TrySound]

@timdorr It's actually can be handled by better treeshaking. Rollup solves a lot but we still stuck on transpiled classes. React hooks will solve the problem with treeshakability.

[pshrmn]

I don't think that this issue is directly related to those link changes. I disabled module concatenation to see the unused exports, and the history exports aren't marked as unused even though the components that use them are unused.

// react-router-dom exports
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "a", function() { return BrowserRouter; });
/* unused harmony export HashRouter */
/* unused harmony export Link */
/* unused harmony export NavLink */

// react-router exports
/* unused harmony export MemoryRouter */
/* unused harmony export Prompt */
/* unused harmony export Redirect */
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "a", function() { return Route; });
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "b", function() { return Router; });
/* unused harmony export StaticRouter */
/* unused harmony export Switch */
/* unused harmony export generatePath */
/* unused harmony export matchPath */
/* unused harmony export withRouter */
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "c", function() { return context; });

// history exports
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "a", function() { return createBrowserHistory; });
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "b", function() { return createHashHistory; });
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "d", function() { return createMemoryHistory; });
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "c", function() { return createLocation; });
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "f", function() { return locationsAreEqual; });
/* unused harmony export parsePath */
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "e", function() { return createPath; });

[TrySound]

@pshrmn I solved the problem of react-router-dom only. There is a work to be done in react-router.
https://github.com/ReactTraining/react-router/blob/master/packages/react-router/.size-snapshot.json#L8

[mjackson]

Thank you for bringing this to our attention, @billyjanitsch. I was planning on verifying that tree-shaking was working as I'd hoped before shipping 4.4 final. When we do ship 4.4, we need to have a recommended strategy for people to be able to eliminate all pieces of the router they aren't using. If that means using Rollup, I'm fine with that, though it'd be nice if we could support webpack 4 too.

[billyjanitsch]

When we do ship 4.4, we need to have a recommended strategy for people to be able to eliminate all pieces of the router they aren't using.

Glad to hear this! 😊 And thanks for considering.

If that means using Rollup, I'm fine with that, though it'd be nice if we could support webpack 4 too.

That would be unfortunate, IMO. Rollup is relatively common for bundling libs, but webpack is overwhelmingly used to bundle apps, whereas react-router is almost exclusively consumed by apps rather than libs.

Could you explain why the solution of not bundling the ESM builds into a single file isn't really being considered? As I see it, it would allow both webpack and Rollup to fully tree-shake the library without any special config, and it wouldn't incur any bundle size increase or initialization cost. The upsides of bundling into a single file (mentioned by @TrySound in #6465 (comment)) seem inaccurate or comparatively small:

It's bundled faster.

Only to a trivial degree -- the time difference between bundling 1 vs ~10 files is negligible.

More consistency in imports.

I'm not suggesting that react-router should support deep imports. Only the top-level package should be imported from (import {BrowserRouter} from 'react-router-dom', never import BrowserRouter from 'react-router-dom/esm/BrowserRouter'). Unless I misunderstand what's meant by "consistency in imports"?

Less import statements.

Modern bundlers (including webpack 4) inline ESM imports/exports at bundle time without any glue code, so the number of import statements has no effect on the resulting bundle. It just provides more surface area for bundlers to do module-level tree-shaking, which is a good thing.

Internals in splitted files are hidden which is the most important.

I see why this is a nice guarantee to have, but as long as the readme makes clear that users shouldn't access library internals (no import foo from 'pkg/lib/foo'), I don't think it's a must-have.

Do these upsides really outweigh not being able to support tree-shaking in webpack and/or having to rely on brittle optimization patterns (e.g. remembering never to use default props or do other static property assignment)?

[TrySound]

@billyjanitsch

Unless I misunderstand what's meant by "consistency in imports"?

I meant these statements in the same project. They are inconsistent.

import {BrowserRouter} from 'react-router-dom'
import Link from 'react-router-dom/Link'

Modern bundlers (including webpack 4) inline ESM imports/exports at bundle time without any glue code, so the number of import statements has no effect on the resulting bundle. It just provides more surface area for bundlers to do module-level tree-shaking, which is a good thing.

Less import statements to write for users. What do you mean by "module-level tree-shaking"? Path imports? It's not treeshaking. It's user execution. I know it as material-ui user.

Do these upsides really outweigh not being able to support tree-shaking in webpack and/or having to rely on brittle optimization patterns (e.g. remembering never to use default props or do other static property assignment)?

Importing from library paths is not treeshaking. Treeshaking is machine analyses - those brittle optimization patterns.

The main idea of those patterns is simple: "use only functions". defaultProps is useless part in the world with hooks.