Skip to content

fix: do not record a deletion when a base value is written back over its draft - #1295

Open
giaBaoJS wants to merge 1 commit into
immerjs:mainfrom
giaBaoJS:fix/spurious-remove-patch-on-base-value-writeback
Open

fix: do not record a deletion when a base value is written back over its draft#1295
giaBaoJS wants to merge 1 commit into
immerjs:mainfrom
giaBaoJS:fix/spurious-remove-patch-on-base-value-writeback

Conversation

@giaBaoJS

@giaBaoJS giaBaoJS commented Sep 5, 2026

Copy link
Copy Markdown

Problem

With enablePatches(), a producer that assigns a property its own base value back and then changes anything else on the same object emits a remove patch for the property it did not touch. Replaying the patch stream drops that property.

import {produceWithPatches, applyPatches, enablePatches} from "immer"
enablePatches()

const child = {id: 1}
const base = {child, other: 0}

const [next, patches] = produceWithPatches(base, d => {
	d.child // read, which drafts the child
	d.child = child // put the base value back
	d.other = 1
})

next // {child: {id: 1}, other: 1}
patches
// [{op: "remove", path: ["child"]},
//  {op: "replace", path: ["other"], value: 1}]

applyPatches(base, patches) // {other: 1}   <- `child` is gone

next is correct. Only the patches are wrong, so the damage lands on whoever replays them: a server applying patches from a client, an undo stack, or applyPatches on a second copy of the state all end up with the property deleted. The inverse patches carry the matching bogus add, so the round trip is lossy in both directions.

It reproduces at any depth ({a: {child, other: 0}} gives remove ["a", "child"]) and in the production build.

Cause

src/core/proxy.ts:185-191. The set trap has a fast path for the "assign the original value back over the draft Immer made for it" case:

// special case, if we assigning the original value to a draft, we can ignore the assignment
const currentState: ProxyObjectState = current?.[DRAFT_STATE]
if (currentState && currentState.base_ === value) {
	state.copy_![prop] = value
	state.assigned_!.set(prop, false)
	return true
}

assigned_ is the record patch generation reads, and false there means "deleted", src/plugins/patches.ts:248:

const op = !assignedValue ? REMOVE : has(base_, key) ? REPLACE : ADD

So the branch that exists precisely to say "this was not a change" is the one that files the property as removed.

The entry is invisible until something else marks the object modified, because generatePatchesAndFinalize only runs for a modified state. That is why the reproduction needs the second, unrelated assignment: without it the state stays unmodified, finalize returns base_, and nobody ever reads assigned_.

This branch is guarded by !state.modified_, and under that guard the get trap only ever drafts a value that satisfies value === peek(state.base_, prop) (src/core/proxy.ts:151). So currentState.base_ === value implies state.base_[prop] === value: the property genuinely still holds its base value. Nothing was assigned and nothing was deleted, so the correct record is no record.

Fix

state.assigned_!.delete(prop) instead of state.assigned_!.set(prop, false). One line.

Under the !state.modified_ guard assigned_ is empty anyway (every writer of a truthy entry calls markChanged first), so in practice this removes an entry that should never have been created rather than erasing a real one.

Arrays are unaffected either way: generateArrayPatches reads assigned_ through a truthiness check (src/plugins/patches.ts:194) rather than treating false as a deletion, so the stale entry was already being skipped there. Only generatePatchesFromAssigned, used for plain objects and Maps, misreads it.

Verification

  • yarn vitest run: 3764 passed, 8 skipped on main, 3772 passed, 8 skipped with this change. The 8 extra results are the 2 new runPatchTests scenarios times the 4 checks that macro generates.
  • yarn test:build (production CJS bundle): 3213 passed on main, 3221 passed with the change, same +8.
  • Counterfactual: restoring only src/core/proxy.ts while keeping the tests turns 4 of those 8 red, for the right reason:
FAIL __tests__/patch.js > assigning a base value back over its own draft > root level > produces the correct patches
AssertionError: expected [ { op: 'remove', ...(1) }, ...(1) ] to deeply equal [ { op: 'replace', ...(2) } ]
+     "op": "remove",
+     "path": [
+       "child",
+     ],

FAIL __tests__/patch.js > assigning a base value back over its own draft > root level > patches are replayable
AssertionError: expected { other: 1 } to deeply equal { child: { id: 1 }, other: 1 }

The produced the correct result check passes on main in both scenarios, which pins the defect to patch generation rather than to the produced value.

  • The same reproduction run directly against dist/cjs/immer.cjs.production.js gives the bogus remove before the change and the correct single replace after, so this is not a source-only artifact.
  • prettier --check clean on both touched files. yarn test:perf runs unchanged.
  • yarn test:flow could not run locally: flow-bin ships an x86-64 binary and fails to spawn on arm64 with Error: spawn Unknown system error -86, before reading any file. No Flow types are touched here.

I put the tests in __tests__/patch.js through the existing runPatchTests macro, so patches are replayable and patches can be reversed come for free and are exactly the invariants that broke.

One related gap I left out

The Map path has no equivalent fast path at all, so map.set(key, baseValue) after map.get(key) marks the map modified and produce returns a new Map where the object and array paths return the base. That is a structural-sharing gap rather than a patch defect (the patches it produces are correct), and fixing it means adding a branch rather than correcting one, so it felt like a separate change. Happy to open a follow up if you want the Map path brought in line.

The set trap has a fast path for assigning a property its own base value
back after Immer drafted it. That path recorded the property in
`assigned_` as `false`, which patch generation reads as a deletion, so a
producer that writes a base value back and then changes anything else on
the same object emits a spurious "remove" patch. Replaying the patches
then drops a property that is still present in the produced result.

The property still holds its base value at that point, so nothing was
assigned and nothing was deleted. Remove the entry instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant