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

applyPatches produces same result, but should it also preserve identity? #909

Open
1 of 2 tasks
faassen opened this issue Feb 23, 2022 · 2 comments
Open
1 of 2 tasks
Labels

Comments

@faassen
Copy link
Contributor

faassen commented Feb 23, 2022

馃檵鈥嶁檪 Question

Consider the following code:

https://codesandbox.io/s/immer-sandbox-forked-t7ghd9?file=/src/index.ts

While patchedState is indeed deep equal to producedState, there is a difference in that produce preserves object identity, but applyPatches does not.

Is this a property that would be useful? I arrived at this during a different route, which may be my real question: when an unshift happens, instead of an add op for path ["items", 0], which I expected, instead the patches include a replace op for the first two items in the array, and then an add op for position ["items", 2]. Applying these patches will result in object identity being broken.

I am experimenting with using Immer for change tracking, and this makes life more complicated: I want to track whether I need to update paths if their indices have shifted, but I don't get sufficient information from the patch to determine that.

Link to repro

https://codesandbox.io/s/immer-sandbox-forked-t7ghd9?file=/src/index.ts

Environment

We only accept questions against the latest Immer version.

  • Immer version: 9.0.12
  • Occurs with setUseProxies(true)
  • Occurs with setUseProxies(false) (ES5 only)
@mweststrate
Copy link
Collaborator

yeah in Immer patches aren't organised in any specific way, nor to they express the original mutation call, rather, they capture what the browser engine decides to do under the hood, and in the case of unshift that'd be a reassignment on every index (so this is storage wise definitely not the most efficient either!). Now trying to reconcile old and new items that might live at arbitrarily different positions would potentially be a quadratic process, and Immer tries to keep both generating and apply patches as cheap as possible.

However, there is nothing to forbid you to apply those patches on an array more efficiently, by e.g. checking if patch[x].value is already existing at state[y].items, by applying the implicit knowledge about the semantics of the program, this could be more generic than a generic solution by Immer.

Note that Immer could in principle intercept the unshift / splice calls on an array (like MobX does) and build efficienter patches out of it, but there are two potential problems here: 1) it wouldn't work on non-proxy environments, 2) patches are only generated in Immer when reading/writing the final result, not when the mutations is done. In this case, it is more inefficient, but in case of multiple reassignments, that strategy is more efficient. So, tradeoffs :)

@BrianHung
Copy link
Contributor

While patchedState is indeed deep equal to producedState, there is a difference in that produce preserves object identity, but applyPatches does not.

The reason for this lies within this PR: #411 Patch values are deeply copied before applying. I actually think deepCopy should either be a flag, or be done by the developer before passing the patches to applyPatches.

Is this a property that would be useful?

I've recently found a use case for this, which is re-applying patches with the same value that exists in the tree should be a no-op. This occurs when you have local optimistic updates, and want to filter by comparing by identities.

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

3 participants