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.

[billyjanitsch]

@TrySound I think there's been a misunderstanding. 🙂 I am not suggesting path imports as a solution. We both agree that path imports are very bad.

Let me make an example repo to show you what I mean by module-level tree-shaking.

[billyjanitsch]

@TrySound I made an example repo to demonstrate. Please compare the multiple-files branch to the single-file branch.

Both branches contain an identical index.js which has import {foo} from './react-router'. This represents a top-level import from a package (similar to import {BrowserRouter} from 'react-router'). There are no "path imports" in either branch.

The difference is that the single-file branch's react-router directory contains only an index.js file which defines and exports foo and bar. This represents the way the react-router ESM build is currently shipped (bundled into a single file).

The multiple-files branch's react-router directory instead contains foo.js and bar.js and an index.js which re-exports foo and bar. This represents the way I think the react-router ESM build should be shipped (left as multiple files).

Comparing the build output of the multiple-files branch to the single-files branch, you'll notice that only the multiple-files branch was able to tree-shake bar. This is because webpack handled the tree-shaking on the module level -- it realized through static analysis that the bar import was unused, so it pruned the bar.js module entirely. (It only does this when {sideEffects: false} is set.) It can't do this when foo and bar are defined in the same file, and Uglify isn't smart enough to tree-shake bar during the DCE phase, so bar is leftover in the single-file branch.

I hope this helps you understand what I mean by module-level tree-shaking. It does not affect how users consume or import from the package, and I am not recommending path imports.

[pshrmn]

I tried converting this to a basic rollup project and that failed to properly tree shake the unused history exports, too.

rollup config

const resolve = require("rollup-plugin-node-resolve");
const commonjs = require("rollup-plugin-commonjs");
const replace = require("rollup-plugin-replace");

module.exports = {
  input: "src/index.js",
  output: {
    file: "dist/bundle.js",
    format: "iife",
    globals: {
      react: "React",
      "react-dom": "ReactDOM"
    }
  },
  external: ["react", "react-dom"],
  plugins: [
    resolve(),
    commonjs({
      include: /node_modules/,
      namedExports: {
        "react-is": ["isValidElementType"]
      }
    }),
    replace({
      "process.env.NODE_ENV": JSON.stringify(process.env.NODE_ENV)
    })
  ]
};

---

My best understanding of this problem is this Webpack issue. react-router-dom imports functions from history, so those get marked as used. Ideally, Webpack realizes that the functions that use those history imports are unused and mark the history imports as unused as well, but I guess that it can't do that yet.

That issue also implies that module concatenation/minimization should fix it (which doesn't appear to be the case here) and that there is a plugin to do "deep scope analysis" to mark the history imports as unused (which doesn't seem to work here), so I really don't know. 😩

I really wish that there were visual tree shaking tools/plugins.

[TrySound]

@pshrmn You need to add uglify or terser to eliminate transpiled classes with pure annotation. Rollup does not handle them. Transpiled classes is evil. Everything will work perfectly with hooks. I will try to optimize react-router better.

[mjackson]

FWIW this is the biggest blocker to releasing 4.4 for me right now. If we don't have an easy solution for people to keep their bundle size down, we should probably revert and tell people to use require('react-router/Link') instead.

[billyjanitsch]

we should probably revert and tell people to use require('react-router/Link') instead.

@mjackson Please don't do this, if possible. This prevents the ESM build from being used ('react-router/Link' points to a CJS file which loads other CJS files, whereas 'react-router' conditionally points to ESM or CJS depending on what the bundler supports). It also makes it easy to accidentally include both the ESM and CJS builds of react-router and/or dependent packages in a bundle (e.g. #6288, #6278).

If we don't have an easy solution for people to keep their bundle size down

What's wrong with the solution I proposed in the OP, detailed in #6464 (comment)? Just stop rolling the ESM build into a single file.

[TrySound]

Yeah, seems like terser/uglify suck in dead code elimination. Transpiled classes suck. We are in the bad state until we will fully migrate to hooks.

[TrySound]

However it's probably not so bad.
Currently (master build) after import * as router from 'react-router-dom there are 32 kb in bundle.
When we import { Route } from 'react-router-dom there are 16.5 kb in bundle.
I just found a way to reduce the latest number to 13.9 kb.

I didn't dig into final bundle but I'm almost sure that all left code will be always consumed by users. So there's no reason to achieve 100% treeshakability right now.

[mjackson]

What's wrong with the solution I proposed in the OP

My apologies for not addressing your comments propertly until now, @billyjanitsch. I think building ESM to multiple files could definitely be a workable solution for now, at least until webpack is able to tree-shake single file builds better.

Question for you: does the multiple file solution work for people who are consuming the CommonJS build? e.g. using a bundler that doesn't understand ES modules, like an earlier version of webpack? Or do those people still need to use relative path imports to simulate the benefits of tree-shaking?

[TrySound]

Don't hurry with rewriting everything back please. Working on solution.

[jadbox]

Has this stalled out? Seems activity has dropped out here..

[TrySound]

@jadbox react-router and react-router-dom are treeshakable. Not fully but left pieces are always included by user.

[Stupidism]

@TrySound Do you mean we can use tree-shaking plugins like lodash-webpack-plugin for react-router, too?

[TrySound]

@Stupidism No, lodash-webpack-plugin cherry pick imports (import { map } from 'lodash' -> import map from 'lodash/map'). Treeshaking is different. It is a kind of minification process which for example checks that function is not used and does not include it in output bundle.

Treeshaking works out of the box with webpack and rollup.

[mjackson]

I believe all our code is tree-shakable for now, except for Router which still has one static method. All other static methods/properties are now gone as of 9d278b4.

I'm going to push a new beta release today (beta 7) that should be fully tree-shakable. Please let me know if you see any issues, @billyjanitsch.

Closing for now.