Repository: immerjs/immer
Pull request #401 (2019-08-02 14:53:32)

Several fixes to patch generation
Fixed an issue where adding and then deleting a property generated the wrong patches; in Proxy implementation, it generated an `add` patch with value `undefined`. 

Fixed an issue where replaying patches on a draft in a producer, where in the middle there was an 'entire state replacement' patch, this new state could not be returned, as both the draft would be modified and a new state would be returned. Nice benefit of this fix is that any patch before a replacement won't even be replayed. 


Patch:
diff --git a/src/immer.js b/src/immer.js
index d60c78b..f0c7a89 100644
--- a/src/immer.js
+++ b/src/immer.js
@@ -121,12 +121,25 @@ export class Immer {
 		assign(this, value ? modernProxy : legacyProxy)
 	}
 	applyPatches(base, patches) {
-		// Mutate the base state when a draft is passed.
+		// If a patch replaces the entire state, take that replacement as base
+		// before applying patches
+		let i
+		for (i = patches.length - 1; i >= 0; i--) {
+			const patch = patches[i]
+			if (patch.path.length === 0 && patch.op === "replace") {
+				base = patch.value
+				break
+			}
+		}
+
 		if (isDraft(base)) {
+			// N.B: never hits if some patch a replacement, patches are never drafts
 			return applyPatches(base, patches)
 		}
 		// Otherwise, produce a copy of the base state.
-		return this.produce(base, draft => applyPatches(draft, patches))
+		return this.produce(base, draft =>
+			applyPatches(draft, patches.slice(i + 1))
+		)
 	}
 	/** @internal */
 	processResult(result, scope) {
diff --git a/src/patches.js b/src/patches.js
index 4463b7b..0d5b3d5 100644
--- a/src/patches.js
+++ b/src/patches.js
@@ -1,4 +1,5 @@
 import {each} from "./common"
+import {createDraft} from "./immer"
 
 export function generatePatches(state, basePath, patches, inversePatches) {
 	Array.isArray(state.base)
@@ -95,41 +96,39 @@ 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
-		if (path.length === 0 && patch.op === "replace") {
-			draft = patch.value
-		} else {
-			let base = draft
-			for (let i = 0; i < path.length - 1; i++) {
-				base = base[path[i]]
-				if (!base || typeof base !== "object")
+		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
-			}
-			const key = path[path.length - 1]
-			switch (patch.op) {
-				case "replace":
+		}
+		const key = path[path.length - 1]
+		switch (patch.op) {
+			case "replace":
+				base[key] = patch.value
+				break
+			case "add":
+				if (Array.isArray(base)) {
+					// TODO: support "foo/-" paths for appending to an array
+					base.splice(key, 0, patch.value)
+				} else {
 					base[key] = patch.value
-					break
-				case "add":
-					if (Array.isArray(base)) {
-						// TODO: support "foo/-" paths for appending to an array
-						base.splice(key, 0, patch.value)
-					} else {
-						base[key] = patch.value
-					}
-					break
-				case "remove":
-					if (Array.isArray(base)) {
-						base.splice(key, 1)
-					} else {
-						delete base[key]
-					}
-					break
-				default:
-					throw new Error("Unsupported patch operation: " + patch.op)
-			}
+				}
+				break
+			case "remove":
+				if (Array.isArray(base)) {
+					base.splice(key, 1)
+				} else {
+					delete base[key]
+				}
+				break
+			default:
+				throw new Error("Unsupported patch operation: " + patch.op)
 		}
 	}
 	return draft
diff --git a/src/proxy.js b/src/proxy.js
index f4caefd..33d7ed4 100644
--- a/src/proxy.js
+++ b/src/proxy.js
@@ -156,6 +156,9 @@ function deleteProperty(state, prop) {
 	if (peek(state.base, prop) !== undefined || prop in state.base) {
 		state.assigned[prop] = false
 		markChanged(state)
+	} else if (state.assigned[prop]) {
+		// if an originally not assigned property was deleted
+		delete state.assigned[prop]
 	}
 	if (state.copy) delete state.copy[prop]
 	return true


Test patch:
diff --git a/__tests__/patch.js b/__tests__/patch.js
index fa38724..32aa526 100644
--- a/__tests__/patch.js
+++ b/__tests__/patch.js
@@ -4,6 +4,8 @@ import produce, {setUseProxies, applyPatches} from "../src/index"
 jest.setTimeout(1000)
 
 function runPatchTest(base, producer, patches, inversePathes) {
+	let resultProxies, resultEs5
+
 	function runPatchTestHelper() {
 		let recordedPatches
 		let recordedInversePatches
@@ -24,17 +26,24 @@ function runPatchTest(base, producer, patches, inversePathes) {
 		test("patches can be reversed", () => {
 			expect(applyPatches(res, recordedInversePatches)).toEqual(base)
 		})
+
+		return res
 	}
 
 	describe(`proxy`, () => {
 		setUseProxies(true)
-		runPatchTestHelper()
+		resultProxies = runPatchTestHelper()
 	})
 
 	describe(`es5`, () => {
 		setUseProxies(false)
-		runPatchTestHelper()
+		resultEs5 = runPatchTestHelper()
+		test("ES5 and Proxy implementation yield same result", () => {
+			expect(resultEs5).toEqual(resultProxies)
+		})
 	})
+
+	return resultProxies
 }
 
 describe("applyPatches", () => {
@@ -414,3 +423,109 @@ describe("simple delete", () => {
 		]
 	)
 })
+
+describe("patch compressions yields correct results", () => {
+	let p1, p2
+	runPatchTest(
+		{},
+		d => {
+			d.x = {test: true}
+		},
+		(p1 = [
+			{
+				op: "add",
+				path: ["x"],
+				value: {
+					test: true
+				}
+			}
+		])
+	)
+	runPatchTest(
+		{x: {test: true}},
+		d => {
+			delete d.x
+		},
+		(p2 = [
+			{
+				op: "remove",
+				path: ["x"]
+			}
+		])
+	)
+	const res = runPatchTest(
+		{},
+		d => {
+			applyPatches(d, [...p1, ...p2])
+		},
+		[]
+	)
+
+	expect(res).toEqual({})
+})
+
+describe("change then delete property", () => {
+	const res = runPatchTest(
+		{
+			x: 1
+		},
+		d => {
+			d.x = 2
+			delete d.x
+		},
+		[
+			{
+				op: "remove",
+				path: ["x"]
+			}
+		]
+	)
+	test("valid result", () => {
+		expect(res).toEqual({})
+	})
+})
+
+test("replaying patches with interweaved replacements should work correctly", () => {
+	const patches = []
+	const s0 = {x: 1}
+
+	const s1 = produce(
+		s0,
+		draft => {
+			draft.x = 2
+		},
+		p => {
+			patches.push(...p)
+		}
+	)
+
+	const s2 = produce(
+		s1,
+		draft => {
+			return {x: 0}
+		},
+		p => {
+			patches.push(...p)
+		}
+	)
+
+	const s3 = produce(
+		s2,
+		draft => {
+			draft.x--
+		},
+		p => {
+			patches.push(...p)
+		}
+	)
+
+	expect(s3).toEqual({x: -1}) // correct result
+	expect(applyPatches(s0, patches)).toEqual({x: -1}) // correct replay
+
+	// manual replay on a draft should also be correct
+	expect(
+		produce(s0, draft => {
+			return applyPatches(draft, patches)
+		})
+	).toEqual({x: -1})
+})