Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible improvements to tree shaking and bundle size #500

Closed
markerikson opened this issue Jan 9, 2020 · 11 comments
Closed

Possible improvements to tree shaking and bundle size #500

markerikson opened this issue Jan 9, 2020 · 11 comments

Comments

@markerikson
Copy link
Contributor

🐛 Bug Report

I have three observations about potential issues with Immer's bundle size and tree-shakeability:

  • Immer went from 4.5K min+gz in 4.x, to around 6K min+gz in v5.x. v5.2.0 is a slight improvement over 5.1.0, but still bigger than 4.x. See https://bundlephobia.com/result?p=immer@5.2.0 for size comparisons. I assume this is due to the extra code for Set/Map handling.
  • Redux Toolkit depends on Immer, and we've noted that several of our dependencies prevent RTK from being correctly tree-shakeable. Someone ran a Webpack report to get indications of why Webpack couldn't tree-shake RTK, and it report that Immer has a reference to process inside, which forces Webpack to bail out of tree-shaking. Reference: Tree shaking is hindered by dependencies reduxjs/redux-toolkit#78 (comment) . The process reference appears to be part of a minification check, at
    typeof process !== "undefined"
    . This does show up in dist/immer.js in the published package.
  • Immer does not appear to have a sideEffects: false flag in its package.json, which Webpack (and other bundlers?) need to fully calculate shakeability: https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free .

I don't know how much improvement you'd get by adding sideEffects: false and removing the process reference, but it might be worth looking into.

I also realize that Immer's internals may not be entirely shakeable anyway, similar to how React is kind of all-or-nothing. Still, given that v5.x is bigger than v4.x, it'd be nice to see if it can be brought back down.

Link to repro

N/A, given that this is not a runtime issue. See linked items in the "report" section for details.

To Reproduce

Use Immer as currently published.

Observed behavior

Immer's size in v5 is larger than v4, and does not appear to be shakeable.

Expected behavior

Webpack would report that Immer is shakeable, and hopefully the final included size would be a bit smaller.

Environment

We only accept bug reports against the latest Immer version.

  • Immer version: v5.2.0
@mweststrate
Copy link
Collaborator

having a small webpack test project that only imports produce would probably a great way to test the actual size

@giggo1604
Copy link

having a small webpack test project that only imports produce would probably a great way to test the actual size

I created a repo following the webpack guide:

https://webpack.js.org/guides/tree-shaking/ https://github.com/giggo1604/immer-treeshake

build output is the following:

$ yarn build
yarn run v1.21.1
$ webpack
Hash: 0319a607aced1cb18598
Version: webpack 4.41.6
Time: 124ms
Built at: 02/17/2020 6:21:29 PM
  Asset      Size  Chunks             Chunk Names
main.js  21.5 KiB       0  [emitted]  main
Entrypoint main = main.js
[2] ./src/index.js + 1 modules 344 bytes {0} [built]
    | ./src/index.js 244 bytes [built]
    | ./src/math.js 95 bytes [built]
    + 2 hidden modules
Done in 0.64s.

looks like everything is ending up in the bundle

@mweststrate
Copy link
Collaborator

Already started, see #536 for current progress. Now using import-size to automate that measurement process. Seems we will drop the initial size of Immer from 6KB to 3-3.5 KB

@mweststrate
Copy link
Collaborator

@markerikson (and others) the new version can be tried as immer@6.0.0-alpha.3. Installation instructions can be found here: https://github.com/immerjs/immer/blob/multi-bundle/docs/installation.md#pick-your-immer-version

@markerikson
Copy link
Contributor Author

Awesome! I'll give it a shot tonight.

Looking at those docs, I think for RTK we'd probably enable ES5 support by default, but leave out Maps and Patches to save space. We'd then point users at the Immer docs with instructions to manually import Immer and flip the switches themselves if necessary.

Which actually brings up an interesting question: if one bit of code has already attempted to enable a plugin like ES5, is there a way to then turn around and disable that support? Say I'm a Redux user who's only targeting modern browsers. If RTK has called enableES5() internally, is there a way for an app to override that and turn it off?

@markerikson
Copy link
Contributor Author

Follow-up question: so the alpha just came out now. What's your ETA on a potential final 6.0 release?

RTK 1.3 is in alpha right now as well. We're still trying to nail down some bits of behavior on our new APIs and make sure things are thoroughly documented. I'd assumed Immer 6 wouldn't be ready until after RTK 1.3 was out, but if you think Immer 6 will be out in the next few days, I might hold off so we could get that in too.

@mweststrate
Copy link
Collaborator

mweststrate commented Feb 25, 2020 via email

@mweststrate
Copy link
Collaborator

mweststrate commented Feb 25, 2020 via email

@markerikson
Copy link
Contributor Author

Yeah, that's what I figured.

This becomes an interesting question: assume that we have enough users who would want that behavior to enable it as the default (between RN and old browsers) and have the larger bundle size, or opt out and give instructions on how to enable it?

Given that I'd like to add this in an RTK minor, I'd say we'd probably want to go for enabling ES5 by default so that no one's code breaks (since that's how it behaves now), and consider maybe changing that in a notional future RTK 2.0.

Thanks for the info!

@mweststrate
Copy link
Collaborator

mweststrate commented Feb 25, 2020 via email

@mweststrate
Copy link
Collaborator

Released as 6.0.0. (minimal) size has been reduced from 6.1 to 3.1 kb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants