The produce function mutates base parameter after applying replace patch on property of type Map

[colesam]

🐛 Bug Report

Immer's produce function begins to mutate state after a specific set of patches are applied to my state.

My state looks like this:

const state = {
  stocks: new Map([
    [ "INTC", new Stock({ name: "INTC", ticker: "INTC", priceHistory: [100, 120] }) ]
    // ...
  ])
}

The Stock class does have the immerable symbol set to true.

If I apply a patch to state that includes a replace operation with a path of ["stocks"], immer's produce function will begin to mutate any changes to the stocks in the future.

Link to repro

This sandbox reproduces the issue: https://codesandbox.io/s/kind-hypatia-0gk5v?file=/src/index.js

To Reproduce

It will be easier to just look at the sandbox, but I'll do my best to verbally describe the issue:

1. Call enableMapSet() and enablePatches().
2. Create an object that has a key of type Map. This Map's values should be a complex object with [immerable] = true.
3. Use produce to create a mutated copy of state.
4. Use produce to reset the mutated copy back to it's original by replacing the attribute with a new Map. Save patches to a variable.
5. Use applyPatches on the mutated copy to create another copy of the original.
6. Use produce on this new copy of the original. Any changes made inside produce will mutate the object created in step number 5

It's difficult to describe the exact reproduction steps which is why I included the code sandbox.

I would like to note that this issue does not reproduce if the map has plain objects instead of complex objects.

Observed behavior

The produce function mutates the object passed as the base parameter, if that object was created with applyPatches under a specific set of circumstances described above.

Expected behavior

The produce function should not mutate the object passed as the base parameter.

Environment

We only accept bug reports against the latest Immer version.

• Immer version:
• I filed this report against the latest version of Immer - version 8.0.2
• Occurs with setUseProxies(true)
• Occurs with setUseProxies(false) (ES5 only)

[mweststrate]

You are not using your Stock class at all

…

On Thu, Mar 18, 2021 at 12:44 PM Samuel Cole ***@***.***> wrote: 🐛 Bug Report Immer's produce function begins to mutate state after a specific set of patches are applied to my state. My state looks like this: const state = { stocks: new Map([ [ "INTC", new Stock({ name: "INTC", ticker: "INTC", priceHistory: [100, 120] }) ] // ... ]) } The Stock class *does* have the immerable symbol set to true. If I apply a patch to state that includes a replace operation with a path of ["stocks"], immer's produce function will begin to mutate any changes to the stocks in the future. Link to repro This sandbox reproduces the issue: https://codesandbox.io/s/kind-hypatia-0gk5v?file=/src/index.js To Reproduce It will be easier to just look at the sandbox, but I'll do my best to verbally describe the issue: 1. Call enableMapSet() and enablePatches(). 2. Create an object that has a key of type Map. This Map's values should be a complex object with [immerable] = true. 3. Use produce to create a mutated copy of state. 4. Use produce to reset the mutated copy back to it's original by replacing the attribute with a new Map. Save patches to a variable. 5. Use applyPatches on the mutated copy to create another copy of the original. 6. Use produce on this new copy of the original. Any changes made inside produce will mutate the object created in step number 5 It's difficult to describe the exact reproduction steps which is why I included the code sandbox. I would like to note that this issue *does not* reproduce if the map has plain objects instead of complex objects. Observed behavior The produce function mutates the object passed as the base parameter, if that object was created with applyPatches under a specific set of circumstances described above. Expected behavior The produce function should not mutate the object passed as the base parameter. Environment We only accept bug reports against the latest Immer version. - *Immer version:* - I filed this report against the *latest* version of Immer - version 8.0.2 - Occurs with setUseProxies(true) - Occurs with setUseProxies(false) (ES5 only) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#768>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBH7DR43LOUUEKFBQVDTEHYSFANCNFSM4ZMRQ3MQ> .

[colesam]

You are not using your Stock class at all

In the sandbox I'm not but in my application the Stock class has a lot more properties, methods, and getters.

I stripped it down to the bare minimum to reproduce in the sandbox, so although the Stock class might seem pointless in the sandbox, it's necessary in my application.

If you'd like I can expand the sandbox example so that the Stock class gets used, but it doesn't affect the reproduction of the issue.

[mweststrate]

Please do, otherwise we aren't solving the right problem, you're description talks about immerable but your reproduction isnt using at this point, so it's impossible to conclude from the repro that immerable isnt respected.

…

On Thu, 18 Mar 2021, 14:15 Samuel Cole, ***@***.***> wrote: You are not using your Stock class at all In the sandbox I'm not but in my application the Stock class has a lot more properties, methods, and getters. I stripped it down to the bare minimum to reproduce in the sandbox, so although the Stock class might seem pointless in the sandbox, it's necessary in my application. If you'd like I can expand the sandbox example so that the Stock class gets used, but it doesn't affect the reproduction of the issue. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#768 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBBH6HEQBMWPRTN2E5DTEIDJDANCNFSM4ZMRQ3MQ> .

[colesam]

I updated the codesandbox.

I added a pushPrice method to the Stock class and changed:

const state4 = produce(state3, (s) => {
  s.stocks.get("INTC").priceHistory.push(150);
});

to

const state4 = produce(state3, (s) => {
  s.stocks.get("INTC").pushPrice(150);
});

Issue remains that the stock in state3 is mutated after the produce call.

To clarify, I don't know for sure if the issue is with immerable classes, or with maps that have immerable classes as values, or both. I added another test case to the example to make it more clear that it isn't just the immerable classes that continue to be referentially equal (and mutated) across calls to produce, but also the map that holds those classes.

[colesam]

Please let me know if I can provide more details. Essentially the situation I have in my application is this:

Multiple users connect to each other with WebRTC and one is the host who mutates state. The host mutates state exclusively using produce and emits the patches to all the peers. The host never applies these patches and is not encountering any errors.

The errors for the peers came up once I began to memoize my react components, because the reference of these stocks was remaining constant across patches, failing to trigger rerenders.

The Stock class has a lot of getters and methods that simplify mutation (similar to the pushPrice example I added). Their presence doesn't really affect the issue as far as I can tell (it reproduces either way), but if you want I can include a lot more code in the codesandbox.

I tried debugging what happens for this snippet:

const state4 = produce(state3, (s) => {
  s.stocks.get("INTC").pushPrice(150);
});

From what I could tell it's following the same path through produce that it usually does. The proxy appears to get created correctly, but when the changes are applied to the proxy, base is mutated. If you can give me any hint of what to look for I'm happy to keep digging in as this is really causing a huge brick wall for my application at the moment.

// Only plain objects, arrays, and "immerable classes" are drafted.
if (isDraftable(base)) {
	const scope = enterScope(this)
	const proxy = createProxy(this, base, undefined)
	let hasError = true
	try {
		result = recipe(proxy) // base is mutated by this call (line 96 of immerClass.ts)
		hasError = false
	} finally {
		// finally instead of catch + rethrow better preserves original stack
		if (hasError) revokeScope(scope)
		else leaveScope(scope)
	}
       // ...

[mweststrate]

it looks to be specifically an issue with the applyPatches, it might be that it doesn't either handle the map, the class, or the combo of those two correctly. I'd be great if the test could be narrowed down a bit further first. Until it is just one single failing test, no cloning involved, etc :) It might be that having a Map as patch value itself causes a problem, not fully sure

…

On Fri, Mar 19, 2021 at 11:53 AM Samuel Cole ***@***.***> wrote: Please let me know if I can provide more details. Essentially the situation I have in my application is this: Multiple users connect to each other with WebRTC and one is the host who mutates state. The host mutates state exclusively using produce and emits the patches to all the peers. The host never applies these patches and is not encountering any errors. The errors for the peers came up once I began to memoize my react components, because the reference of these stocks was remaining constant across patches, failing to trigger rerenders. The Stock class has a lot of getters and methods that simplify mutation (similar to the pushPrice example I added). Their presence doesn't really affect the issue as far as I can tell (it reproduces either way), but if you want I can include a lot more code in the codesandbox. I tried debugging what happens for this snippet: const state4 = produce(state3, (s) => { s.stocks.get("INTC").pushPrice(150);}); From what I could tell it's following the same path through produce that it usually does. The proxy *appears* to get created correctly, but when the changes are applied to the proxy, base is mutated. If you can give me any hint of what to look for I'm happy to keep digging in as this is really causing a huge brick wall for my application at the moment. // Only plain objects, arrays, and "immerable classes" are drafted.if (isDraftable(base)) { const scope = enterScope(this) const proxy = createProxy(this, base, undefined) let hasError = true try { result = recipe(proxy) // base is mutated by this call hasError = false } finally { // finally instead of catch + rethrow better preserves original stack if (hasError) revokeScope(scope) else leaveScope(scope) } // ... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#768 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDBI2K3AZHCQ7CPX7LTEM3LVANCNFSM4ZMRQ3MQ> .

[colesam]

I'll see what I can accomplish over the weekend.

[colesam]

I did some additional tests to try to isolate the issue as much as possible. It turns out that this problem has less to do with maps and more to do with immerable classes.

Please see: https://github.com/colesam/immer-issue-768

To summarize, it looks like the issue only occurs when immerable classes are part of the value of a replacement patch.

When the replacement patch is not replacing the root of the base object, and there is an immerable class in the value of the patch, the issue will reproduce (for example if it is an item of an array, or a property of an object). When the replacement patch has a path of [], the issue does not reproduce.

Not exactly sure where to go from here. While debugging it looked like the proxy object in produce was being created correctly, but clearly it's not if it's allowing the target to be mutated.

[mweststrate]

Thanks for the detailed reproduction! Trimmed it a little further down and found the culprit. I didn't verify all test cases, but 9.0.1 should fix the, or one of the, underlying issue.

[mweststrate]

🎉 This issue has been resolved in version 9.0.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀