Skip to content

fix: emit replace patches for indices dropped by shrinking length - #1356

Open
RClaudiuM wants to merge 1 commit into
immerjs:mainfrom
RClaudiuM:fix/1355-length-shrink-patches
Open

RClaudiuM wants to merge 1 commit into
immerjs:mainfrom
RClaudiuM:fix/1355-length-shrink-patches

Conversation

@RClaudiuM

Copy link
Copy Markdown

Fixes #1355

What

With enablePatches(), shrinking a draft array by assigning length and then growing it back to at least its base length produced no replace patch (and no inverse patch) for the truncated indices. So applyPatches(base, patches) kept the stale values, and the inverse patches did not restore the base. The same recipe with splice(0) was already correct.

Why

generateArrayPatches only emits a replace for an index below base_.length when assigned_ has it (or allIndicesReassigned_ is set). splice removes elements through deleteProperty, which marks each index. Assigning length goes through the set trap for "length" only, so the dropped indices were never marked.

Change

In the array set trap, when length shrinks, mark the dropped indices that exist in the base as assigned. Only indices below base_.length are marked, since those are the only ones the replace loop looks at. If the array stays short, nothing changes: the replace loop only runs up to the shorter length, and the dropped indices still get remove patches.

Tests

New cases in __tests__/patch.js (through runPatchTests, so they also check replay and reversal):

  • length = 0 then d[1] = 2 (the case from the issue)
  • length = 0 then length = 2
  • length = 1 then push(4, 5) (control, already passing)
  • length = 0 then the same values written back (control: no patches)

Before the fix, the first two fail (6 assertions). After, all pass.

Repro from the issue, run against the build:

before:
patches [{"op":"add","path":[1],"value":2}]
inverse [{"op":"remove","path":[1]}]
replay  [ 1, 2 ]
undo    [ <1 empty item> ]

after:
patches [{"op":"replace","path":[0],"value":undefined},{"op":"add","path":[1],"value":2}]
inverse [{"op":"replace","path":[0],"value":1},{"op":"remove","path":[1]}]
replay  [ undefined, 2 ]
undo    [ 1 ]

Checks run locally (Node 24): yarn test (src, build and flow), yarn coverage, yarn test:perf, prettier --check.

Assigning a smaller `length` to a draft array truncated `copy_` without
marking the dropped indices as assigned. If the array then grew back to at
least its base length, generateArrayPatches skipped those indices, so the
patches did not replay to the result and the inverse patches did not restore
the base. Mark the dropped base indices as assigned in the array set trap, as
deleting them one by one already does.

Fixes immerjs#1355

This branch has not been deployed

No deployments
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.

Shrinking an array with length = and growing it back loses the patches for the truncated indices

1 participant