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
Open
fix: do not record a deletion when a base value is written back over its draft#1295giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
With
enablePatches(), a producer that assigns a property its own base value back and then changes anything else on the same object emits aremovepatch for the property it did not touch. Replaying the patch stream drops that property.nextis correct. Only the patches are wrong, so the damage lands on whoever replays them: a server applying patches from a client, an undo stack, orapplyPatcheson a second copy of the state all end up with the property deleted. The inverse patches carry the matching bogusadd, so the round trip is lossy in both directions.It reproduces at any depth (
{a: {child, other: 0}}givesremove ["a", "child"]) and in the production build.Cause
src/core/proxy.ts:185-191. Thesettrap has a fast path for the "assign the original value back over the draft Immer made for it" case:assigned_is the record patch generation reads, andfalsethere means "deleted",src/plugins/patches.ts:248: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
generatePatchesAndFinalizeonly runs for a modified state. That is why the reproduction needs the second, unrelated assignment: without it the state stays unmodified,finalizereturnsbase_, and nobody ever readsassigned_.This branch is guarded by
!state.modified_, and under that guard thegettrap only ever drafts a value that satisfiesvalue === peek(state.base_, prop)(src/core/proxy.ts:151). SocurrentState.base_ === valueimpliesstate.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 ofstate.assigned_!.set(prop, false). One line.Under the
!state.modified_guardassigned_is empty anyway (every writer of a truthy entry callsmarkChangedfirst), so in practice this removes an entry that should never have been created rather than erasing a real one.Arrays are unaffected either way:
generateArrayPatchesreadsassigned_through a truthiness check (src/plugins/patches.ts:194) rather than treatingfalseas a deletion, so the stale entry was already being skipped there. OnlygeneratePatchesFromAssigned, used for plain objects and Maps, misreads it.Verification
yarn vitest run: 3764 passed, 8 skipped onmain, 3772 passed, 8 skipped with this change. The 8 extra results are the 2 newrunPatchTestsscenarios times the 4 checks that macro generates.yarn test:build(production CJS bundle): 3213 passed onmain, 3221 passed with the change, same +8.src/core/proxy.tswhile keeping the tests turns 4 of those 8 red, for the right reason:The
produced the correct resultcheck passes onmainin both scenarios, which pins the defect to patch generation rather than to the produced value.dist/cjs/immer.cjs.production.jsgives the bogusremovebefore the change and the correct singlereplaceafter, so this is not a source-only artifact.prettier --checkclean on both touched files.yarn test:perfruns unchanged.yarn test:flowcould not run locally:flow-binships an x86-64 binary and fails to spawn on arm64 withError: spawn Unknown system error -86, before reading any file. No Flow types are touched here.I put the tests in
__tests__/patch.jsthrough the existingrunPatchTestsmacro, sopatches are replayableandpatches can be reversedcome 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)aftermap.get(key)marks the map modified andproducereturns 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.