Confusing behavior with nested produce calls

[markerikson]

🐛 Bug Report

I've got a scenario where nested produce calls are not generating the results I expect. The inner produce call works okay, but I would expect that when the draft from the outer produce is passed as the initial state to the inner produce, that changes to the inner draft would be reflected in the outer draft as well.

Prior related issues: #377 , #433

Specifically:

• I am using Redux Toolkit's createSlice and createReducer, which call produce internally, and wrap the provided "case reducers" so that they can mutate
• Within a case reducer, I am calling a second function that itself uses Immer internally. The case reducer has received a draft as its state argument, and is passing that draft to the second function.
• The second function receives the outer draft, passes it as the state arg to produce, and then calls a third function that actually does the mutating.

The call stack looks like (roles on the left, actual names in sandbox on the right):

• createSlice/Reducer // createSlice({name: "books"})

  • produce (first) // internal to Redux Toolkit
    • caseReducer(draft1) // addOne reducer in slice
      • adapterFunction(draft1) // createStateAdapter
        • produce(draft1) (second) // called from createStateAdapter
          • mutatorFunction(draft2) // addOneMutably
            • assorted mutations of draft 2 // inside addOneMutably

• Inside of mutatorFunction / addOneMutably, I can see that draft2 has been mutated

• produce(draft1) returns a plain JS object that correctly matches the results of draft2

• Inside of caseReducer / addOne, I can see draft1. However, after the call to adapterFunction(draft1) / createStateAdapter completes, I would expect that draft1 reflects the mutations that were done to draft2. Instead, draft1 is unchanged.

Link to repro

https://codesandbox.io/s/young-violet-5ozps

To Reproduce

Steps to reproduce the behavior:

Run the linked CodeSandbox

Observed behavior

A description of what behavior you observed and consider faulty.

The outer draft was not mutated to match the changes to the inner draft.

Expected behavior

A clear and concise description of what you expected to happen.

Mutations to the inner draft would punch through and also be applied to the outer draft.

It's very likely that I have mistaken expectations with that behavior, but if so, I would appreciate clarification on what the actual intended behavior is.

Environment

We only accept bug reports against the latest Immer version.

• Immer version: : 4.0.2, because Redux Toolkit is currently locked to Immer@^4.0.1 (due to the growth in size of Immer 5.x, and the fact that we don't need the Map/Set changes)
• Occurs with setUseProxies(true)
• Occurs with setUseProxies(false) (ES5 only): N/A

[mweststrate]

There have been quite some bug fixes in this area recently. What happens if you do run this test with Immer locked to the latest version?

[markerikson]

Yeah, I tried updating my local dev copy of Redux Toolkit to Immer 5.3.x and re-run the test with this code in it, and I saw the same behavior. (Also found out that apparently 5.3.x requires TS 3.7+, which was a bit of a surprising jump).

Let me see if my understanding of the intended behavior is correct or not:

If I take the draft from an outer produce function, pass draft1 as the state arg to an inner produce, and then mutate draft2 inside the inner produce, should those changes get reflected in the outer draft as well?

[mweststrate]

Yes, that assumption is correct (IIRC lol); where it often goes wrong if the return of a produce is picked up and assigned to something, because then you get indeed partial results. If you manage to put this into a test that doesn't require RSK (probably by picking the the relevant parts of create slice) I'd gladly dive deeper into it (actual debugging in CS is a bit cumbersome. so if it is copy pasteable it into our own test suite that will be simpler). Or feel free to put a PR where we verify that RSK is working correctly, that would work as well and would be great in preventing we break RSK in the future unaware (probably we would need to something magic by making sure it picks immer from source, some linking or relative-deps magic could help with that)

…

On Tue, Feb 11, 2020 at 3:58 PM Mark Erikson ***@***.***> wrote: Yeah, I tried updating my local dev copy of Redux Toolkit to Immer 5.3.x and re-run the test with this code in it, and I saw the same behavior. (Also found out that apparently 5.3.x requires TS 3.7+, which was a bit of a surprising jump). Let me see if my understanding of the *intended* behavior is correct or not: If I take the draft from an outer produce function, pass draft1 as the state arg to an inner produce, and then mutate draft2 inside the inner produce, should those changes get reflected in the outer draft as well? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#533?email_source=notifications&email_token=AAN4NBFBFYCVBRX4SEMSLB3RCLDMDA5CNFSM4KSZRTKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELM6X5I#issuecomment-584707061>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBD5GSLBE3HUC47342TRCLDMDANCNFSM4KSZRTKA> .

[mweststrate]

Just to confirm my own memory: This works indeed:

it("can be nested", () => {
	const base = { x : 1 }

	const p1 = produce(s => {
		s.x++
	})

	const p2 = produce(s => {
		s.x++
		p1(s)
		s.x++
	})

	expect(p2(base)).toEqual({
		x: 3
	})
})

However, changing it to this fails:

	const p2 = produce(s => {
		s.x++
		s = p1(s)
		s.x++ // s != the draft here so this won't be seen by anyone
	})

(because s will be frozen after p1, if autoFreeze is disabled, the output will be 2, not 3, because at that last line, s is not referring anymore to the draft so that result is returned nowhere. This might be the scenario you are running into)

[markerikson]

Interesting, because I was actually assuming my initial assumption was wrong :)

And yes, the CSB could be reduced further. I was trying to replicate the current code flow as much as possible by copy-pasting things from the codebase and testing.

I think the scenario I'm dealing with is just a bit different than what you're showing there. I'm not re-assigning s = p1(s) in this case and then trying to further mutate the returned value.

I'll try to take a stab at minimizing the repro case in the next day or two.

Appreciate the response! (especially given that the filed issue isn't strictly against immer@latest).

[markerikson]

Afraid I haven't had a chance to try to put together a more minimal repro of this yet.

I did find an alternative implementation that seems to work as expected.

The inner logic in my code was previously calling produce() (or as we name it in RTK, createNextState() unconditionally.

I saw that Immer exports an isDraft method, and was able to change its logic to:

if(isDraft(state)) {
    // run mutating logic here right away
    callbackWithMutatingLogic()
    return state
} else {
    return createNextState(state, callbackWithMutatingLogic)
}

That results in the desired/expected behavior in a unit test.

I'd still like to come back and file a better repro here, but my immediate focus is trying to get these new RTK APIs past alpha and out the door.

[mweststrate]

Revisit this one after the next Immer release, as soon things in this area will be cleaned up in the next version

[mweststrate]

Would you mind checking if this still happens in 6.0.0?

[mweststrate]

Closing this issue as there hasn't been any activity for a while, so assuming everything is ok now :)