Repository: immerjs/immer
Pull request #413 (2019-08-28 07:54:51)

fix: ensure patches are reusable
In the original `applyPatches`  implementation, in the `replace` and `add` switch cases, new values are assigned using `base[key] = patch.value`. 
In this way, if the values are objects, then they are assigned by reference. 
Later `applyPatches` operations may change the assigned values, and hence the value fields in the patches will also be modified.

I modified the `applyPatches` implementation and added an additional new test case.



Patch:
diff --git a/src/patches.js b/src/patches.js
index 0d5b3d5..2c803e4 100644
--- a/src/patches.js
+++ b/src/patches.js
@@ -95,29 +95,46 @@ function generateObjectPatches(state, basePath, patches, inversePatches) {
 	})
 }
 
-export function applyPatches(draft, patches) {
-	// First, find a patch that replaces the entire state, if found, we don't have to apply earlier patches and modify the state
-	for (let i = 0; i < patches.length; i++) {
-		const patch = patches[i]
-		const {path} = patch
+// used to clone patch to ensure original patch is not modified
+const clone = obj => {
+	if (obj === null || typeof obj !== "object") return obj
+
+	if (Array.isArray(obj)) return obj.map(clone)
+
+	const cloned = {}
+	for (const key in obj) cloned[key] = clone(obj[key])
+
+	return cloned
+}
+
+export const applyPatches = (draft, patches) => {
+	for (const patch of patches) {
+		const {path, op} = patch
+		const value = clone(patch.value)
+
 		if (!path.length) throw new Error("Illegal state")
+
 		let base = draft
 		for (let i = 0; i < path.length - 1; i++) {
 			base = base[path[i]]
 			if (!base || typeof base !== "object")
-					throw new Error("Cannot apply patch, path doesn't resolve: " + path.join("/")) // prettier-ignore
+				throw new Error("Cannot apply patch, path doesn't resolve: " + path.join("/")) // prettier-ignore
 		}
+
 		const key = path[path.length - 1]
-		switch (patch.op) {
+		switch (op) {
 			case "replace":
-				base[key] = patch.value
+				// if value is an object, then it's assigned by reference
+				// in the following add or remove ops, the value field inside the patch will also be modifyed
+				// so we use value from the cloned patch
+				base[key] = value
 				break
 			case "add":
 				if (Array.isArray(base)) {
 					// TODO: support "foo/-" paths for appending to an array
-					base.splice(key, 0, patch.value)
+					base.splice(key, 0, value)
 				} else {
-					base[key] = patch.value
+					base[key] = value
 				}
 				break
 			case "remove":
@@ -128,8 +145,9 @@ export function applyPatches(draft, patches) {
 				}
 				break
 			default:
-				throw new Error("Unsupported patch operation: " + patch.op)
+				throw new Error("Unsupported patch operation: " + op)
 		}
 	}
+
 	return draft
 }


Test patch:
diff --git a/__tests__/patch.js b/__tests__/patch.js
index 32aa526..7fc1fef 100644
--- a/__tests__/patch.js
+++ b/__tests__/patch.js
@@ -1,5 +1,9 @@
 "use strict"
-import produce, {setUseProxies, applyPatches} from "../src/index"
+import produce, {
+	setUseProxies,
+	applyPatches,
+	produceWithPatches
+} from "../src/index"
 
 jest.setTimeout(1000)
 
@@ -82,6 +86,28 @@ describe("applyPatches", () => {
 			applyPatches({}, [patch])
 		}).toThrowErrorMatchingSnapshot()
 	})
+	it("applied patches cannot be modified", () => {
+		// see also: https://github.com/immerjs/immer/issues/411
+		const s0 = {
+			items: [1]
+		}
+
+		const [s1, p1] = produceWithPatches(s0, draft => {
+			draft.items = []
+		})
+
+		const replaceValueBefore = p1[0].value.slice()
+
+		const [s2, p2] = produceWithPatches(s1, draft => {
+			draft.items.push(2)
+		})
+
+		applyPatches(s0, [...p1, ...p2])
+
+		const replaceValueAfter = p1[0].value.slice()
+
+		expect(replaceValueAfter).toStrictEqual(replaceValueBefore)
+	})
 })
 
 describe("simple assignment - 1", () => {