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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks done
colesam opened this issue Mar 18, 2021 · 10 comments
Closed
3 tasks done
Labels

Comments

@colesam
Copy link

@colesam colesam commented Mar 18, 2021

馃悰 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
Copy link
Collaborator

@mweststrate mweststrate commented Mar 18, 2021

@colesam
Copy link
Author

@colesam colesam commented Mar 18, 2021

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
Copy link
Collaborator

@mweststrate mweststrate commented Mar 18, 2021

@colesam
Copy link
Author

@colesam colesam commented Mar 18, 2021

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
Copy link
Author

@colesam colesam commented Mar 19, 2021

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
Copy link
Collaborator

@mweststrate mweststrate commented Mar 19, 2021

@colesam
Copy link
Author

@colesam colesam commented Mar 19, 2021

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

@colesam
Copy link
Author

@colesam colesam commented Mar 20, 2021

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
Copy link
Collaborator

@mweststrate mweststrate commented Mar 20, 2021

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
Copy link
Collaborator

@mweststrate mweststrate commented Mar 20, 2021

馃帀 This issue has been resolved in version 9.0.1 馃帀

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

No branches or pull requests

2 participants