fix: key inserted array indices by name in the array-methods plugin - #1283
Merged
markerikson merged 1 commit intoAug 5, 2026
Merged
Conversation
`handleInsertedValues` recorded inserted indices in `assigned_` as numbers,
but `assigned_` is keyed by property name everywhere else, because the proxy
traps only ever see strings. Patch generation looks indices up with
`assigned_?.get(i.toString())`, so a numeric key was never found.
The added indices loop in `generateArrayPatches` covers everything past
`base_.length` regardless of `assigned_`, which hid the mismatch for a plain
push. It surfaced when an insert reuses an index that already existed in the
base, e.g. dropping the oldest entry and appending a new one:
const [next, patches] = produceWithPatches(["a", "b", "c"], draft => {
draft.pop()
draft.push("z")
})
// next -> ["a", "b", "z"]
// patches -> [] (expected: replace /2 with "z")
The result was correct but no patch described it, so undo/redo and state
sync built on the patch stream silently dropped the change.
Stringifying the index also keeps the key consistent for the finalization
lookup in `handleCrossReference`, which reads `assigned_` with the same key
this function passes it.
markerikson
approved these changes
Aug 5, 2026
Contributor
|
🎉 This PR is included in version 11.1.16 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
The bug
With
enableArrayMethods(), an array update that removes an element and inserts a new one produces the correct next state but no patches at all, so anything built on the patch stream (undo/redo, state sync) silently loses the change.Reproduced against the published
immer@11.1.15:The same happens for
draft.length = n; draft.push(x)and for asplicethat inserts over an index the base already had. Without the plugin the patch is generated correctly, so this is specific to the array-methods path.Root cause
assigned_is keyed by property name everywhere else in immer — the proxy traps only ever receive strings, so an array index is stored as"2".handleInsertedValuesstored it as the number2:Patch generation reads it back by name (
src/plugins/patches.ts):get("2")never finds the key2, so the index looks untouched and noreplacepatch is emitted. The mismatch stayed hidden for a plainpushbecause the added indices loop ingenerateArrayPatchescovers everything pastbase_.lengthunconditionally; it only shows up once an insert lands on an index that already existed in the base.isRelocatedBaseRefinsrc/core/proxy.tsreads the same map by name and was equally blind to these keys.The fix
Stringify the index so the plugin follows the same key convention as the traps. The key is also what
handleCrossReferencecarries into the finalization lookup, so both readers now agree on it.Verification
push()cases in__tests__/base.js. That file runs every case both with and without the plugin, and the new test fails only in the plugin variant before this change.yarn test(vitest+test:build): 3689 passing, no changes to existing expectations.test:flowcould not run locally —flow-binhas no darwin-arm64 binary — but nothing here touches the flow types.yarn test:perfshows no movement.applyPatches(base, patches) ≍ nextandapplyPatches(next, inverse) ≍ base. Scenarios where the forward patches failed to reproduce the result dropped from 57 to 21, with no scenario newly broken.The 21 that remain are a separate defect with a different cause — the plugin's in-place removals (
pop,shift,splice) never record the removed indices inassigned_, so an index that is vacated and then re-covered by a laterlengthwrite or out-of-range index assignment still goes unpatched. I left it out to keep this change to one root cause; happy to follow up on it if you'd like.