search

flitbit / diff

Javascript utility for calculating deep difference, capturing changes, and applying changes across objects; for nodejs and the browser.
MIT License
2.99k stars 213 forks source link

applyChange() possibly modifies change object #142

Closed soulmerge closed 5 years ago

soulmerge commented 5 years ago

The current implementation of applyChange() copies a reference to the change object's rhs property in line 379. Any changes to the target object also changes the change object afterwards. Example:

const target = {};
const change1 = { kind: "N": path: ["foo"], rhs: {} };
const change2 = { kind: "N": path: ["foo", "bar"], rhs: "bug" };
applyChange(target, true, change1);
applyChange(target, true, change2);
console.log(change1.rhs.bar) // prints "bug"

I am currently working around this issue by deep-cloning my change objects before applying them.

I would issue a pull request using clone-deep, but I'm not sure if that is in the best interest of this library, as it has no dependencies at the moment.

flitbit commented 5 years ago

If possible, don't add any dependencies. I understand your impulse to use an immutable strategy, but I'm confused about the bug you perceive. The behavior you describe is working as designed because diff doesn't change the mutability of anything, it simply makes the structures match.

When you apply change1 to the target, target receives the value of rhs as the value of the property foo.

When you apply change2 to the target, target.foo receives the value of rhs as the value of bar.

Since the value of target.foo references the same object referenced by change1.rhs, your observation is accurate, but so is the implementation of applyChange.

If your code requires that you store change sets and apply them at a later time, and you want the change sets unmolested, simply clone your change sets before applying them.