diff --git a/__tests__/array-methods-removed.js b/__tests__/array-methods-removed.js new file mode 100644 index 00000000..c48a4186 --- /dev/null +++ b/__tests__/array-methods-removed.js @@ -0,0 +1,136 @@ +import { + Immer, + enableArrayMethods, + enablePatches, + freeze, + isDraft +} from "../src/immer" +import {clearPlugin, PluginArrayMethods} from "../src/internal" + +enablePatches() + +for (const enabled of [false, true]) { + for (const autoFreeze of [false, true]) { + describe(`removed array values: plugin=${enabled}, autoFreeze=${autoFreeze}`, () => { + beforeEach(() => { + if (enabled) enableArrayMethods() + else clearPlugin(PluginArrayMethods) + }) + afterEach(() => clearPlugin(PluginArrayMethods)) + + for (const method of ["pop", "shift", "splice"]) { + test(`${method} returns a draft that can move between arrays`, () => { + for (const frozen of [false, true]) { + const immer = new Immer({autoFreeze}) + const base = {todo: [{done: false, details: {count: 0}}], done: []} + if (frozen) freeze(base, true) + let patches, inverse + const next = immer.produce( + base, + draft => { + const item = + method === "splice" + ? draft.todo.splice(0, 1)[0] + : draft.todo[method]() + expect(isDraft(item)).toBe(true) + draft.done.push(item) + item.done = true + item.details.count++ + }, + (p, i) => { + patches = p + inverse = i + } + ) + expect(base.todo[0]).toEqual({done: false, details: {count: 0}}) + expect(next).toEqual({ + todo: [], + done: [{done: true, details: {count: 1}}] + }) + expect(next.done[0]).not.toBe(base.todo[0]) + expect(next.done[0].details).not.toBe(base.todo[0].details) + expect(immer.applyPatches(base, patches)).toEqual(next) + expect(immer.applyPatches(next, inverse)).toEqual(base) + } + }) + } + + test("preserves holes when splicing sparse arrays", () => { + const immer = new Immer({autoFreeze}) + const base = {items: [, , {done: false}], done: []} + const next = immer.produce(base, draft => { + const removed = draft.items.splice(0, 3) + expect(0 in removed).toBe(false) + expect(1 in removed).toBe(false) + draft.done.push(removed[2]) + removed[2].done = true + }) + expect(base.items[2].done).toBe(false) + expect(next.done[0].done).toBe(true) + }) + + test("reuses a child draft that was read before removal", () => { + const immer = new Immer({autoFreeze}) + const base = {items: [{done: false}], done: []} + const next = immer.produce(base, draft => { + const before = draft.items[0] + const removed = draft.items.pop() + expect(removed).toBe(before) + draft.done.push(removed) + removed.done = true + }) + expect(base.items[0].done).toBe(false) + expect(next.done[0].done).toBe(true) + }) + + test("does not reread getters outside the removed position", () => { + const immer = new Immer({autoFreeze}) + let reads = 0 + const items = [{done: false}, {done: false}] + Object.defineProperty(items, 0, { + get() { + if (++reads > 1) throw new Error("getter read twice") + return {done: false} + }, + enumerable: true, + configurable: true + }) + const next = immer.produce({items, done: []}, draft => { + const item = draft.items.pop() + draft.done.push(item) + item.done = true + }) + expect(reads).toBe(1) + expect(items[1].done).toBe(false) + expect(next.done[0].done).toBe(true) + }) + + test("preserves empty no-op return values and structural sharing", () => { + const immer = new Immer({autoFreeze}) + const base = {items: []} + const next = immer.produce(base, draft => { + expect(draft.items.pop()).toBeUndefined() + expect(draft.items.shift()).toBeUndefined() + expect(draft.items.splice(0, 0)).toEqual([]) + }) + expect(next).toBe(base) + }) + + test("keeps newly inserted objects undrafted when removed", () => { + const immer = new Immer({autoFreeze}) + for (const method of ["pop", "shift", "splice"]) { + const external = {done: false} + immer.produce({items: []}, draft => { + draft.items.push(external) + const item = + method === "splice" + ? draft.items.splice(0, 1)[0] + : draft.items[method]() + expect(item).toBe(external) + expect(isDraft(item)).toBe(false) + }) + } + }) + }) + } +} diff --git a/src/plugins/arrayMethods.ts b/src/plugins/arrayMethods.ts index 1bb60a02..cec2dfeb 100644 --- a/src/plugins/arrayMethods.ts +++ b/src/plugins/arrayMethods.ts @@ -10,11 +10,9 @@ import { /** * Methods that directly modify the array in place. - * These operate on the copy without creating per-element proxies: - * - `push`, `pop`: Add/remove from end - * - `shift`, `unshift`: Add/remove from start (marks all indices reassigned) - * - `splice`: Add/remove at arbitrary position (marks all indices reassigned) - * - `reverse`, `sort`: Reorder elements (marks all indices reassigned) + * All except `splice` operate on the internal copy. `pop` and `shift` + * first read their removed value through the normal proxy. `splice` + * uses the normal proxy throughout to preserve native argument coercion. */ type MutatingArrayMethod = | "push" @@ -68,8 +66,9 @@ export type ArrayOperationMethod = MutatingArrayMethod | NonMutatingArrayMethod * This plugin overrides array methods to avoid unnecessary Proxy creation during iteration, * significantly improving performance for array-heavy operations. * - * **Mutating methods** (push, pop, shift, unshift, splice, sort, reverse): - * Operate directly on the copy without creating per-element proxies. + * **Mutating methods**: + * Most operate directly on the copy; pop and shift first draft removed values. + * splice uses the normal proxy to preserve removed drafts and argument coercion. * * **Non-mutating methods** fall into categories: * - **Subset operations** (filter, slice, find, findLast): Return draft proxies - mutations track @@ -341,6 +340,30 @@ export function enableArrayMethods() { // Enter operation mode - this flag tells the proxy's get trap to return // base values instead of creating nested proxies during iteration const method = originalMethod as ArrayOperationMethod + if (method === "splice") { + // Splice arguments can coerce or mutate the array. Let the native + // method read through the proxy without normalizing them twice. + const previousOperation = state.operationMethod + state.operationMethod = undefined + try { + return Reflect.apply(Array.prototype[method], state.draft_, args) + } finally { + state.operationMethod = previousOperation + } + } + if (method === "pop" || method === "shift") { + // Draft only the removed item before mutating the internal copy. + const length = latest(state).length + if (length > 0) { + const previousOperation = state.operationMethod + state.operationMethod = undefined + try { + void state.draft_[method === "pop" ? length - 1 : 0] + } finally { + state.operationMethod = previousOperation + } + } + } enterOperation(state, method) try { @@ -353,53 +376,6 @@ export function enableArrayMethods() { if (REORDERING_METHODS.has(method)) { return handleReorderingOperation(state, method, args) } - - if (method === "splice") { - const insertCount = args.length > 2 ? args.length - 2 : 0 - - // Detect no-op calls before creating a copy so structural - // sharing is preserved (see handleSimpleOperation). A splice - // removes nothing when called with no arguments, when the - // normalized start is at/past the end, or when the delete - // count coerces to less than 1. splice(start) with a single - // argument removes everything from start onwards, so it must - // not match. Exotic arguments that slip through are caught - // by the post-copy check below. - if (insertCount === 0) { - const length = latest(state).length - if ( - args.length === 0 || - normalizeSliceIndex(args[0] ?? 0, length) === length || - (args.length > 1 && !(args[1] >= 1)) - ) { - return [] - } - } - - prepareCopy(state) - // The start index must be resolved against the length the array - // had before the splice, like the native method does - const lengthBefore = state.copy_!.length - const res = state.copy_!.splice( - ...(args as [number, number, ...any[]]) - ) - - // Backstop for exotic arguments the pre-check above missed: - // if nothing was removed and nothing inserted, the array is - // unchanged and must not be marked as modified. - if (res.length === 0 && insertCount === 0) { - return res - } - - markArrayChanged(state) - markAllIndicesReassigned(state) - // Handle cross-references for inserted values (args from index 2+) - if (insertCount > 0) { - const startIndex = normalizeSliceIndex(args[0] ?? 0, lengthBefore) - handleInsertedValues(state, startIndex, args.slice(2)) - } - return res - } } else { // Handle non-mutating methods return handleNonMutatingOperation(state, method, args) diff --git a/website/docs/array-methods.md b/website/docs/array-methods.md index e2c0d948..7f4dafc3 100644 --- a/website/docs/array-methods.md +++ b/website/docs/array-methods.md @@ -27,7 +27,7 @@ This adds approximately **2KB** to your bundle size. ## Mutating Methods -These methods modify the array in-place and operate directly on the draft's internal copy without creating per-element proxies: +These methods modify the draft array in-place. `push()`, `unshift()`, `sort()`, and `reverse()` operate directly on its internal copy. `pop()` and `shift()` first draft only the removed element, then remove it from the internal copy. `splice()` uses the normal draft proxy throughout to preserve native argument coercion and return safe removed drafts; its performance therefore matches the standard proxy path. | Method | Returns | Description | | ----------- | ---------------- | ------------------------------------- | @@ -39,6 +39,8 @@ These methods modify the array in-place and operate directly on the draft's inte | `sort()` | The draft array | Sorts elements in place | | `reverse()` | The draft array | Reverses the array in place | +Removed objects originating in the base state remain drafts, so they can be moved to another draft array and updated without changing the base. Newly inserted external objects retain their normal undrafted behavior. + ```javascript import {produce, enableArrayMethods} from "immer"