Producers not eventually producing a modified value still spawn a new value

[ymeine]

🐛 Bug Report

Using on an array a producer which first modifies a value but later on reverts it to the original one still keeps the draft marked as "modified" and therefore spawns a new array, making the output not strictly equal to the input.

To Reproduce

Execute that:

const immer = require('immer');

const original = [1];
const modified = immer.produce(original, function (array) {
	array[0] = 2; // modify it
	array[0] = 1; // revert it back to original value
});
console.log(modified === original); // outputs "false"

Expected behavior

From the concept of immutability, I expect conceptually equal values to be strictly equal in practice. In other words: a reference pointing to an array of value [1] should always be strictly equal to another reference pointing to an array of value [1].

I understand that using an API like immer, it is difficult to track that down across different calls to produce. e.g.

const original = [1];

const modified1 = immer.produce(original, function (array) { array[0] = 2; });
console.log(modified1 === original); // outputs "false" as expected

const modified2 = immer.produce(modified1, function (array) { array[0] = 1; });
console.log(modified2 === original); // outputs "false" while we could expect "true", since they actually have the same value ([1]), but I understand this is difficult to track down

However, I would have expected it to be possible inside the same call to produce:

const original = [1];

const attempt1 = immer.produce(original, function (array) { array[0] = 1; });
console.log(attempt1 === original); // outputs "true" as expected

const attempt2 = immer.produce(
	original,
	function (array) {
		array[0] = 2; // marks the value as "modified"
		array[0] = 1; // doesn't mark it back to "not modified"
	},
	function (patches) {
		console.log(patches); // this outputs an empty list, and that is expected!
		// This can be used as workaround outside to reassign the output of `produce` to the base value if patches list is empty,
		// so that I can achieve my goal of having the same reference
	}
);
console.log(attempt2 === original); // outputs "false" and that's not really expected

Link to repro

Immer sandbox - CodeSandbox (open the console).

Environment

• Immer version: 2.1.5
• Occurs with setUseProxies(true)
• Occurs with setUseProxies(false) (ES5 only)

PS: identified piece of code: immer/proxy.js at master · immerjs/immer, once modified once, no chance to check if value is back to the original. Anyways, there is markUnchanged counterpart, and with complex hierarchies I understand marking back as "not modified" could be complex though, since it doesn't necessarily means the parent is not modified anymore

[aleclarson]

I'm changing this to feature request, since the docs never claim that produce should detect this.

PRs welcome ❤️

[agierlicki]

Correct me if I'm wrong but isn't this behaviour the whole point of immutability? You modified the object (even though you did it in a way that doesn't actually change the data) so the object is not the same anymore. If you would perform your steps one after the other you wouldn't expect them to be equal again afterwards, would you?

[mweststrate]

The point of immutability is that if objects are reference equal, they are deep equal. That inverse statement, deep equal objects are reference equal, is a nice feat. But it is neither a problem nor a necessity for immutability. So if objects, after undoing changes, are reference equal again, that is technically perfectly fine, although hard to achieve in practice. (There are FP languages / libraries that do achieve this, really cool!). Whether that is the expected or surprising behavior, I don't really know; in most cases it probably doesn't (or shoudn't) really matter.

…

On Tue, Apr 30, 2019 at 4:08 PM Alexander Gierlicki < ***@***.***> wrote: Correct me if I'm wrong but isn't this behaviour the whole point of immutability? You modified the object (even though you did it in a way that doesn't actually change the data) so the object is not the same anymore. If you would perform your steps one after the other you wouldn't expect them to be equal again afterwards, would you? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#360 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBGNRLRV3PQKMYWRHODPTBHFBANCNFSM4HI4AHRQ> .

[ymeine]

Well, if two references are equal, they will always respect deep equality too, because modifications are done on the same object. So the simple point of immutability is to prevent the ability to modify the content of an object instance, that's all. That way, you can say that if your reference was corresponding to a given value in time, it will always correspond to the same value anytime. With that definition, immer respects the contract I agree. And so would a deep copy followed by alterations. (then I might have missed the point of immer, except maybe for the ability to record patches?)

I understand the will to keep references to objects that won't change, it has a great value. I realize I mixed technical and functional aspects. So I will still have to go with deep comparisons to check if two references refer to the same object conceptually (even if different objects in practice). Also, as far as I remember, even Immutable.js (for example) won't return equal references for similar object, you still have to use the method is to check for conceptual equality.

Then I think my bug-report-which-is-actually-a-feature-request is not really relevant. And indeed, it would be quite difficult to achieve (maintaining a pool of immutable objects in the course of the program execution, without leaks, I'm not so motivated myself). Only the specific case of modifications inside the same produce call, but that might make the code more complex for only a specific, maybe rare use case.

Then I'll close my issue.

Thanks a lot for your time and consideration :)

[aleclarson]

With that definition, immer respects the contract I agree. And so would a deep copy followed by alterations. (then I might have missed the point of immer, except maybe for the ability to record patches?)

Immer also provides mutable drafts (which allows for imperative mutations that result in immutable copies) and structural sharing (which means shallow copies are preferred).

[ymeine]

Immer also provides mutable drafts (which allows for imperative mutations that result in immutable copies) and structural sharing (which means shallow copies are preferred).

Oh I see... Indeed I understand now. A deep copy first would always copy deeply nested objects, resulting is some maybe unnecessary copies if only shallow levels are altered. On the other hand, doing conditional copies would add complex logic to the code, while immer keeps it simple instead (as if a big initial deep copy had been made).

Thank you for highlighting this aspect :)