Immer 8

[markerikson]

Looks like Immer 8 just came out, and it switches to always auto-freezing in production.

It also looks like Immer 7 was slower than 6, but switching auto-freezing on actually improves perf.

We need to look into this and figure out when/how to upgrade. Can we do this in an RTK minor?

• https://github.com/immerjs/immer/releases/tag/v8.0.0
• Idea: always freeze + freeze export immerjs/immer#687
• Performance of mutating large collections: arrays vs objects immerjs/immer#649
• Immer 7 is 8 times slower than immer 6 immerjs/immer#681

[rangigo]

I will try to migrate in my local fork, if it's working properly I will push it and make a PR (⌐■_■)

[markerikson]

My big questions here are:

• How much of a meaningful perf impact does leaving auto-freezing on have in some various Redux app use cases?
• Can we actually change the behavior to leave auto-freeze on (ie, the new Immer default), and consider that an RTK minor version? Or is it a big enough change that we should consider it "breaking" ourselves, and specifically call setAutoFreeze(false) in the same way that we turn on ES5 support automatically?

[rangigo]

• Do we have some sort of existing benchmarks for these kind of cases in Redux repo?
• And for your second point I do not think it is that big of a change. But I have to make some proper test cases when I finish the migration. Will try to take into accounts your questions. Just a heads-up this is my first attempt at contributing to OSS 👹

[phryneas]

Hmm.

* How much of a meaningful perf impact does leaving auto-freezing on have in some various Redux app use cases?

We probably won't know that unless we see it :/

* Can we actually change the behavior to leave auto-freeze on (ie, the new Immer default), and consider that an RTK minor version?  Or is it a big enough change that we should consider it "breaking" ourselves, and specifically call `setAutoFreeze(false)` in the same way that we turn on ES5 support automatically?

So, I've looked around a bit. Immer only freezes stuff that is isDraftable, with this check:

export function isDraftable(value: any): boolean {
	if (!value) return false
	return (
		isPlainObject(value) ||
		Array.isArray(value) ||
		!!value[DRAFTABLE] ||
		!!value.constructor[DRAFTABLE] ||
		isMap(value) ||
		isSet(value)
	)
}

So it isn't just freezing everything in the case that someone goes against our advice and puts mutable/class/non-serializable stuff in there.
As for the serializable rest: I can't think of a single case where such a mutation were not a bug.
Which means we wouldn't really change "documented behaviour".
Explicitly calling setAutoFreeze(false) would on the other hand be unexpected for people looking at the version number of the immer they have installed and the official immer documentation.

My suggestion: we wait for the matcher branch, merge #704 and the new immer version in and call that a minor. (We need a new release before RTK-query can be used.)
I'm kinda playing with the thought of implementing #792 myself and take a look how it feels to maybe also get that in.

[markerikson]

I don't think we have any specific perf test cases for Redux or RTK. There's a React-Redux benchmarks repo, but that's really oriented around triggering lots of dispatches to stress-test UI component updates.

I'd say look at some of the Immer PRs to see what benchmarks they might have, and try creating some similar RTK code setups. Not immediately sure how to compare different versions of Immer with RTK here.

And yeah, we're definitely coming up on an RTK 1.5 shortly.

[markerikson]

Fixed by #821 .