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

Immer 10 #1028

Merged
merged 52 commits into from
Apr 17, 2023
Merged

Immer 10 #1028

merged 52 commits into from
Apr 17, 2023

Conversation

mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Mar 23, 2023

Release notes

Overall, there is a rough performance increase of 33% for Immer (and in some cases significantly higher), and the (non gzipped) bundle size has reduced from 16 to 11.5 KB, while the the minimal gzipped import of just produce has remained roughly the same at 3.3 KB.

For more details, see #1015

Migration steps

  1. If you have any enableES5() call, don't migrate
  2. When using getters/ setters icmw plain objects, call useStrictShallowCopy(true) at startup
  3. Replace all default imports: Replace import produce from "immer" with import {produce} from "immer"
  4. Replace all calls to enableAllPlugins() with enablePatches(); enableMapSet(); to be more specific and smoothen future migrations.
  5. If any producer returned a Promise, refactor it to leverage createDraft instead. Roughly:
const newState = await produce(oldState, recipe)

// becomes
const draft  = createDraft(oldState)
await recipe(draft)
const newState = finishDraft(draft)

@coveralls
Copy link

coveralls commented Mar 23, 2023

Pull Request Test Coverage Report for Build 4622958383

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 79 of 79 (100.0%) changed or added relevant lines in 10 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+1.5%) to 99.796%

Files with Coverage Reduction New Missed Lines %
src/utils/errors.ts 1 88.24%
Totals Coverage Status
Change from base Build 4504340943: 1.5%
Covered Lines: 641
Relevant Lines: 642

💛 - Coveralls

__tests__/base.js Outdated Show resolved Hide resolved
src/core/current.ts Outdated Show resolved Hide resolved
src/core/proxy.ts Outdated Show resolved Hide resolved
src/types/index.js.flow Outdated Show resolved Hide resolved
website/docs/api.md Outdated Show resolved Hide resolved
@markerikson
Copy link
Contributor

Just ran a quick check on 9.0.16 vs 10.0.beta, using Vite 4.2.1 + source-map-explorer:

  • 9.0.16: immer/dist/immer.esm.mjs, 10.42KB
  • 10.0.0-beta.4: immer/dist/immer.mjs, 15.78KB

If I hand-inspect the Vite bundle output, I don't see any NODE_ENV references left in there, which is what I would expect - they exist in the ESM artifact, but the minifier should get rid of those.

If it helps at all, I just manually copied and pasted the minified versions of 9.0.16 and 10.0.0-beta.4 out of Vite-built bundles, ran them through Prettier, and am attaching them as a zip - maybe it'll help show where the differences are:

immer-minified-prettified.zip

Per the Map/Set thing: FWIW, we really don't want people using the Map/Set support with RTK because of serialization issues, so at least in our case that is a size increase we don't benefit from. But, I do understand how that can cause confusion and annoyance on your side. (My own preference would be that it still be opt-in somehow if possible.)

@mweststrate
Copy link
Collaborator Author

mweststrate commented Apr 4, 2023 via email

@markerikson
Copy link
Contributor

Yeah, the two most immediate references are here:

We actually had an internal discussion about Map/Set usage again yesterday after seeing your comment, and concluded it's still not sufficiently worth it. It is hypothetically possible to rework the Redux DevTools with one of the new "JSON superset" serialization libs, but it's fairly common for folks to want to persist state in localStorage (often using the redux-persist library), and that would run into serialization issues. There'd also be more nuance to try to explain in the docs, and it's simpler if we leave it at a blanket "Don't!" :)

@mweststrate
Copy link
Collaborator Author

@markerikson I reverted the MapSet opt-in in [email protected], mind doing a check if it solves the bundle size increase?

@markerikson
Copy link
Contributor

@mweststrate yeah, I'll check on that tonight!

@markerikson
Copy link
Contributor

markerikson commented Apr 6, 2023

@mweststrate: Okay, 3 builds of the same RTKQ app, changing only the Immer version:

Version Bundle (min) Bundle (gzip) Sourcemapped file
9.0.16 386.77 kB 122.41 kB 10.42 kB
10.0-beta.4 392.30 kB 123.74 kB 15.78 kB
10.0-beta.6 387.12 kB 122.31 kB 10.76 kB

I'm still generally surprised that 10.x isn't smaller than 9.x, given the changes.

@markerikson
Copy link
Contributor

markerikson commented Apr 7, 2023

@mweststrate : well, you got me hooked :)

I cloned the immer-10 branch and did some poking around. I think there's a few things going on:

  • You're currently building the project with TSDX. That seems to be adding some polyfills, like _extends() (which you can see in the CJS dev build).
  • That in turn is partly because tsconfig.json is set to target: "ES6".

So, those are adding up to a bunch of dead bytes that aren't needed.

Unrelated to the bundle sizes, there's also issues with the current build output in this beta:

  • I know you've been flipping file names back and forth. Right now there's an immer.mjs bundle, but an immer.esm.js.map sourcemap file
  • The sourcemaps themselves seem broken and incorrect. If you try loading some of them into https://evanw.github.io/source-map-visualization/ , you'll see that variables and segments don't line up correctly, making them semi-useless

FWIW, I just switched redux and redux-thunk over to use https://tsup.egoist.dev/ . It's basically like TSDX conceptually, except that it's based on ESBuild + Rollup (and it seems to actually be maintained atm).

I just tried making a local branch that switches over to tsup instead, and after 15-ish minutes I've got a decent part of the output bundles generating. More than that, I've got the minified files shrinking noticeably. In beta.6, immer.cjs.production.min.js is 16542b, and with tsup I've already got it down to 13680b. I also tried generating a minified .mjs artifact for comparison, and that's down to 12941b.

Some other notes here:

  • I see room for some byte shaving tricks. For example, I tried export const getPrototypeOf = Object.getPrototypeOf and deleting the Object. in front of the uses, and that shaved off a few bytes because the variable name gets minified down to just E.
  • I'm 95% set on dropping UMD bundles for Redux 5.0 / RTK 2.0, especially since you can make a pre-built ESM bundle that should be importable in-browser. Might want to consider that here.
  • I'd say you can probably drop the Flow support too, really :)

Sooooo, with all that said:

If you're interested, I can try to do the rest of the TSDX -> tsup conversion, and file that as a PR against this branch.

Thoughts?

@mweststrate
Copy link
Collaborator Author

@markerikson thanks for pointing out tsup! I was already wondering what was the modern bundling setup since tsdx went unmaintained, which was also the reason I didn't want to poke too much in the config. So happy to receive a PR! Please do leave the flow types, as they're useful inside Meta, so I try to keep them roughly up to date :)

@markerikson
Copy link
Contributor

@mweststrate okay, sure! I'll try to put up a PR over the weekend.

@markerikson
Copy link
Contributor

@mweststrate done! See #1032 .

Please let me know if you have any questions or would like me to tweak things further!

@mweststrate
Copy link
Collaborator Author

mweststrate commented Apr 12, 2023 via email

@mweststrate
Copy link
Collaborator Author

@markerikson just published [email protected]

@mweststrate mweststrate merged commit 2ef9a42 into main Apr 17, 2023
@github-actions
Copy link
Contributor

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Immer takes a long time to update the data.
4 participants