fix: reassigning to same value generates new copy with empty patches

Author: oriondean

__tests__/base.js

@@ -179,7 +179,7 @@ function runBaseTest(
 				delete s.a
 				s.a = a
 			})
-			expect(nextState).not.toBe(baseState)
+			expect(nextState).toBe(baseState)
 			expect(nextState).toEqual(baseState)
 		})
 
@@ -3274,8 +3274,7 @@ function runBaseTest(
 				draft.highlight = false
 				draft.highlight = true
 			})
-			// See explanation in issue
-			expect(next1).not.toBe(a)
+			expect(next1).toBe(a)
 
 			const next2 = produce(a, draft => {
 				draft.highlight = true
@@ -3496,7 +3495,7 @@ function runBaseTest(
 			expect(next).toEqual({dots: base.availableStartingDots})
 		})
 
-		it("cannot always detect noop assignments - 0", () => {
+		it("detects noop assignments - 0", () => {
 			const baseState = {x: {y: 3}}
 			const nextState = produce(baseState, d => {
 				const a = d.x
@@ -3505,54 +3504,45 @@ function runBaseTest(
 			expect(nextState).toBe(baseState)
 		})
 
-		it("cannot always detect noop assignments - 1", () => {
+		it("detects noop assignments - 1", () => {
 			const baseState = {x: {y: 3}}
 			const nextState = produce(baseState, d => {
 				const a = d.x
 				d.x = 4
 				d.x = a
 			})
-			// Ideally, this should actually be the same instances
-			// but this would be pretty expensive to detect,
-			// so we don't atm
-			expect(nextState).not.toBe(baseState)
+			expect(nextState).toBe(baseState)
 		})
 
-		it("cannot always detect noop assignments - 2", () => {
+		it("detects noop assignments - 2", () => {
 			const baseState = {x: {y: 3}}
 			const nextState = produce(baseState, d => {
 				const a = d.x
 				const stuff = a.y + 3
 				d.x = 4
 				d.x = a
 			})
-			// Ideally, this should actually be the same instances
-			// but this would be pretty expensive to detect,
-			// so we don't atm
-			expect(nextState).not.toBe(baseState)
+			expect(nextState).toBe(baseState)
 		})
 
-		it("cannot always detect noop assignments - 3", () => {
+		it("detects noop assignments - 3", () => {
 			const baseState = {x: 3}
 			const nextState = produce(baseState, d => {
 				d.x = 3
 			})
 			expect(nextState).toBe(baseState)
 		})
 
-		it("cannot always detect noop assignments - 4", () => {
+		it("detects noop assignments - 4", () => {
 			const baseState = {x: 3}
 			const nextState = produce(baseState, d => {
 				d.x = 4
 				d.x = 3
 			})
-			// Ideally, this should actually be the same instances
-			// but this would be pretty expensive to detect,
-			// so we don't atm
-			expect(nextState).not.toBe(baseState)
+			expect(nextState).toBe(baseState)
 		})
 
-		it("cannot always detect noop assignments - 4", () => {
+		it("cannot always detect noop assignments - 1", () => {
 			const baseState = {}
 			const [nextState, patches] = produceWithPatches(baseState, d => {
 				d.x = 4

__tests__/produce.ts

@@ -177,6 +177,95 @@ it("can apply readonly patches", () => {
 	expect(applyPatches({}, patches)).toEqual({x: 4})
 })
 
+describe("assigning a value and then reverting it, generates no changes", () => {
+	it("top-level", () => {
+		type Item = {date: Date}
+
+		let initialDate = new Date()
+		let data: Item = {date: initialDate}
+
+		const [newData, patches] = produceWithPatches(data, draft => {
+			const dateBefore = draft.date
+			draft.date = new Date()
+			draft.date = dateBefore
+		})
+
+		// Expect no patches and the returned value to be the same reference
+		expect(patches).toEqual([])
+		expect(newData).toBe(data)
+	})
+
+	it("top-level pathological case", () => {
+		type Item = {number: Number}
+
+		let data: Item = {number: -0}
+
+		const [newData, patches] = produceWithPatches(data, draft => {
+			draft.number = 1
+			draft.number = +0
+		})
+
+		// Expect no patches and the returned value to be the same reference
+		expect(newData).not.toBe(data)
+		// Bug in patches, should be a patch but none generated
+		expect(patches).toEqual([])
+	})
+
+	it("top-level with adjacent modified draft", () => {
+		type Item = {date: Date; other: {x: number}}
+
+		let initialDate = new Date()
+		let data: Item = {date: initialDate, other: {x: 0}}
+
+		const [newData, patches] = produceWithPatches(data, draft => {
+			const dateBefore = draft.date
+			draft.date = new Date()
+			draft.other.x++
+			draft.date = dateBefore
+		})
+
+		// Expect patches and the returned value not to be the same reference
+		expect(patches).not.toEqual([])
+		expect(newData).not.toBe(data)
+	})
+
+	it("nested in array", () => {
+		type Item = {date: Date}
+
+		let initialDate = new Date()
+		let data: Item[] = [{date: initialDate}]
+
+		const [newData, patches] = produceWithPatches(data, draft => {
+			const element = draft[0]
+			const dateBefore = element.date
+			element.date = new Date()
+			element.date = dateBefore
+		})
+
+		// Expect no patches and the returned value to be the same reference
+		expect(patches).toEqual([])
+		expect(newData).toBe(data)
+	})
+
+	it("nested in map", () => {
+		type Item = {date: Date}
+
+		let initialDate = new Date()
+		let data: Map<number, Item> = new Map([[0, {date: initialDate}]])
+
+		const [newData, patches] = produceWithPatches(data, draft => {
+			const element = draft.get(0)!
+			const dateBefore = element.date
+			element.date = new Date()
+			element.date = dateBefore
+		})
+
+		// Expect no patches and the returned value to be the same reference
+		expect(patches).toEqual([])
+		expect(newData).toBe(data)
+	})
+})
+
 describe("curried producer", () => {
 	it("supports rest parameters", () => {
 		type State = {readonly a: 1}

src/core/proxy.ts

@@ -22,7 +22,11 @@ import {
 	ENUMERABLE,
 	VALUE,
 	isArray,
-	isArrayIndex
+	isArrayIndex,
+	getProxyDraft,
+	each,
+	get,
+	getValue
 } from "../internal"
 
 interface ProxyBaseState extends ImmerBaseState {
@@ -203,6 +207,8 @@ export const objectTraps: ProxyHandler<ProxyState> = {
 		)
 			return true
 
+		if (revertToBaseIfNeeded(state, prop, value)) return true
+
 		// @ts-ignore
 		state.copy_![prop] = value
 		state.assigned_!.set(prop, true)
@@ -314,6 +320,42 @@ function getDescriptorFromProto(
 	return undefined
 }
 
+function revertToBaseIfNeeded(
+	state: ImmerState,
+	prop: PropertyKey,
+	value: any
+): boolean {
+	if (
+		!has(state.base_, prop, state.type_) ||
+		!is(get(state.base_, prop), getValue(value))
+	) {
+		return false
+	}
+
+	state.copy_![prop] = value
+	state.assigned_!.delete(prop)
+
+	if (state.assigned_!.size > 0) return true
+
+	let childModified = false
+	each(state.copy_!, (key, val) => {
+		if (getProxyDraft(val)?.modified_) {
+			childModified = true
+		}
+	})
+	if (childModified) return true
+
+	state.modified_ = false
+	state.copy_ = null
+	state.assigned_ = undefined
+
+	if (state.parent_ && state.key_ !== undefined) {
+		revertToBaseIfNeeded(state.parent_, state.key_, state.base_)
+	}
+
+	return true
+}
+
 export function markChanged(state: ImmerState) {
 	if (!state.modified_) {
 		state.modified_ = true