From 4e4048e9aad449436e55d77e78c6f7e9d55ee556 Mon Sep 17 00:00:00 2001 From: Dean Kerr Date: Fri, 8 May 2026 14:24:59 +0100 Subject: [PATCH] fix: reassigning to same value generates new copy with empty patches --- __tests__/base.js | 32 ++++++---------- __tests__/produce.ts | 89 ++++++++++++++++++++++++++++++++++++++++++++ src/core/proxy.ts | 44 +++++++++++++++++++++- 3 files changed, 143 insertions(+), 22 deletions(-) diff --git a/__tests__/base.js b/__tests__/base.js index 346f73e9..f939a920 100644 --- a/__tests__/base.js +++ b/__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,20 +3504,17 @@ 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 @@ -3526,13 +3522,10 @@ function runBaseTest( 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 @@ -3540,19 +3533,16 @@ function runBaseTest( 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 diff --git a/__tests__/produce.ts b/__tests__/produce.ts index c3913873..246c4360 100644 --- a/__tests__/produce.ts +++ b/__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 = 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} diff --git a/src/core/proxy.ts b/src/core/proxy.ts index bd758cd6..d8c5d4b0 100644 --- a/src/core/proxy.ts +++ b/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 = { ) 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