From 703d2752afe5c517dbee82aa52e71b7d5fd25fb4 Mon Sep 17 00:00:00 2001 From: Fred Kleuver Date: Tue, 5 Dec 2023 06:26:29 +0100 Subject: [PATCH] fix(repeater): duplicate primitive handling, batched mutation fix (#1840) * fix(repeat): fix sort+splice batched operation bug * test(repeat): verify $index duplicate bug, add skipped test for contextual props mutation bug --- .../src/2-runtime/array-observer.spec.ts | 198 ++++++++++++++++-- .../src/3-runtime-html/repeat.batched.spec.ts | 108 ++++++++++ .../3-runtime-html/repeat.duplicates.spec.ts | 94 +++++++++ packages/aurelia/src/index.ts | 2 - .../resources/template-controllers/repeat.ts | 31 +-- packages/runtime/src/index.ts | 2 - .../runtime/src/observation/array-observer.ts | 62 +----- .../src/observation/subscriber-batch.ts | 2 + 8 files changed, 408 insertions(+), 91 deletions(-) create mode 100644 packages/__tests__/src/3-runtime-html/repeat.batched.spec.ts create mode 100644 packages/__tests__/src/3-runtime-html/repeat.duplicates.spec.ts diff --git a/packages/__tests__/src/2-runtime/array-observer.spec.ts b/packages/__tests__/src/2-runtime/array-observer.spec.ts index 77e1573844..132136a53c 100644 --- a/packages/__tests__/src/2-runtime/array-observer.spec.ts +++ b/packages/__tests__/src/2-runtime/array-observer.spec.ts @@ -5,8 +5,6 @@ import { enableArrayObservation, ICollectionSubscriber, IndexMap, - applyMutationsToIndices, - synchronizeIndices, batch, Collection, } from '@aurelia/runtime'; @@ -17,6 +15,7 @@ import { SpySubscriber, } from '@aurelia/testing'; +const compareNumber = (a: number, b: number): number => a - b; export class SynchronizingCollectionSubscriber implements ICollectionSubscriber { public readonly oldArr: unknown[]; public readonly newArr: unknown[]; @@ -30,28 +29,120 @@ public constructor( } public handleCollectionChange(collection: Collection, indexMap: IndexMap): void { - indexMap = applyMutationsToIndices(indexMap); - const newArr = this.newArr; const oldArr = this.oldArr; + const oldArrCopy = oldArr.slice(); - const deleted = indexMap.deletedIndices; - const deletedLen = deleted.length; - let j = 0; - for (let i = 0; i < deletedLen; ++i) { - j = deleted[i] - i; - oldArr.splice(j, 1); + const deleted = indexMap.deletedIndices.sort(compareNumber); + for (let i = 0; i < deleted.length; ++i) { + oldArr.splice(deleted[i] - i, 1); } - const mapLen = indexMap.length; - for (let i = 0; i < mapLen; ++i) { + for (let i = 0; i < indexMap.length; ++i) { if (indexMap[i] === -2) { oldArr.splice(i, 0, newArr[i]); } } - synchronizeIndices(oldArr, indexMap); + for (let i = 0; i < indexMap.length; ++i) { + const source = indexMap[i]; + if (source !== -2) { + oldArr[i] = oldArrCopy[source]; + } + } } + + // 17 tests fail (combined splice+sort) + // public handleCollectionChange(collection: Collection, indexMap: IndexMap): void { + // const newArr = this.newArr; + // const oldArr = this.oldArr; + // const deleted = indexMap.deletedIndices.sort(compareNumber); + // let cursor = 0; + // let dCurr = deleted[cursor] ?? -1; + // let deletes = 0; + // const deleteOffsetMap = Array(indexMap.length + deleted.length).fill(0); + // for (let i = 0; i < deleteOffsetMap.length; ++i) { + // while (i === dCurr) { + // oldArr.splice(dCurr, 1); + // ++deletes; + // if (deleted.length > ++cursor) { + // dCurr = deleted[cursor] - deletes; + // } else { + // dCurr = -1; + // } + // } + // deleteOffsetMap[i] = deletes; + // } + + // let adds = 0; + // const addsOffsetMap = Array(indexMap.length + deleted.length).fill(0); + // for (let i = 0; i < addsOffsetMap.length; ++i) { + // if (indexMap[i] === -2 && indexMap[i - 1] !== -2) { + // let j = i; + // while (indexMap[j] === -2) { + // oldArr.splice(j, 0, newArr[j]); + // ++adds; + // ++j; + // } + // } + // addsOffsetMap[i] = adds; + // } + + // const oldArrCopy = oldArr.slice(); + // for (let i = 0; i < indexMap.length; ++i) { + // const source = indexMap[i]; + // const offset = addsOffsetMap[i] - deleteOffsetMap[i]; + // if (source !== -2 && source + offset !== i) { + // oldArr[i] = oldArrCopy[source + offset]; + // } + // } + // } + + // 16 different tests fail (push + shift / multi splice combi's) + // public handleCollectionChange(collection: Collection, indexMap: IndexMap): void { + // const newArr = this.newArr; + // const oldArr = this.oldArr; + // const deleted = indexMap.deletedIndices.sort(compareNumber); + // let cursor = 0; + // let dCurr = deleted[cursor] ?? -1; + // let deletes = 0; + // const deleteOffsetMap = Array(indexMap.length + deleted.length).fill(0); + // for (let i = 0; i < deleteOffsetMap.length; ++i) { + // while (i === dCurr) { + // oldArr.splice(dCurr, 1); + // ++deletes; + // if (deleted.length > ++cursor) { + // dCurr = deleted[cursor] - deletes; + // } else { + // dCurr = -1; + // } + // } + // deleteOffsetMap[i] = deletes; + // } + + // let adds = 0; + // const addsOffsetMap = Array(indexMap.length + deleted.length).fill(0); + // for (let i = 0; i < addsOffsetMap.length; ++i) { + // if (indexMap[i] === -2 && indexMap[i - 1] !== -2) { + // let j = i; + // while (indexMap[j] === -2) { + // oldArr.splice(j, 0, newArr[j]); + // ++adds; + // ++j; + // } + // } + // addsOffsetMap[i] = adds; + // } + + // const oldArrCopy = oldArr.slice(); + // for (let i = 0; i < indexMap.length; ++i) { + // const source = indexMap[i]; + // const offset = addsOffsetMap[source] - deleteOffsetMap[source]; + // if (source !== -2 && source + offset !== i) { + // oldArr[i] = oldArrCopy[source + offset]; + // } + // } + // } } describe(`2-runtime/array-observer.spec.ts`, function () { @@ -934,6 +1025,50 @@ describe(`2-runtime/array-observer.spec.ts`, function () { arr.sort(desc); }); }); + + describe('combined splice+sort operations', function () { + it('swap outer items + delete inner item', function () { + verifyChanges([S(1), S(2), S(3)], arr => { + arr.reverse(); + arr.splice(1, 1); + }); + }); + + it('delete inner item + swap outer items', function () { + verifyChanges([S(1), S(2), S(3)], arr => { + arr.splice(1, 1); + arr.reverse(); + }); + }); + + it('swap outer items + replace inner item', function () { + verifyChanges([S(1), S(3), S(4)], arr => { + arr.reverse(); + arr.splice(1, 1, S(2)); + }); + }); + + it('replace inner item + swap outer items', function () { + verifyChanges([S(1), S(3), S(4)], arr => { + arr.splice(1, 1, S(2)); + arr.reverse(); + }); + }); + + it('swap outer items + add inner item', function () { + verifyChanges([S(1), S(3), S(4)], arr => { + arr.reverse(); + arr.splice(1, 0, S(2)); + }); + }); + + it('add inner item + swap outer items', function () { + verifyChanges([S(1), S(3), S(4)], arr => { + arr.splice(1, 0, S(2)); + arr.reverse(); + }); + }); + }); }); describe('array w/ 4 item', function () { @@ -1181,6 +1316,13 @@ describe(`2-runtime/array-observer.spec.ts`, function () { }); }); + it('splice the middle item with three new items and sort desc ---', function () { + verifyChanges([S(1), S(2)], arr => { + arr.splice(1, 1, S(3), S(4)); + arr.reverse(); + }); + }); + it('push + reverse', function () { verifyChanges([S(1), S(2), S(3), S(4)], arr => { arr.push(S(5)); @@ -1231,6 +1373,36 @@ describe(`2-runtime/array-observer.spec.ts`, function () { arr.sort(desc); }); }); + + describe('combined splice+sort operations', function () { + it('swap outer items + delete inner items', function () { + verifyChanges([S(1), S(2), S(3), S(4)], arr => { + arr.reverse(); + arr.splice(1, 2); + }); + }); + + it('delete inner items + swap outer items', function () { + verifyChanges([S(1), S(2), S(3), S(4)], arr => { + arr.splice(1, 2); + arr.reverse(); + }); + }); + + it('swap outer items + replace inner items', function () { + verifyChanges([S(1), S(4), S(5), S(6)], arr => { + arr.reverse(); + arr.splice(1, 2, S(2), S(3)); + }); + }); + + it('replace inner items + swap outer items', function () { + verifyChanges([S(1), S(4), S(5), S(6)], arr => { + arr.splice(1, 2, S(2), S(3)); + arr.reverse(); + }); + }); + }); }); }); diff --git a/packages/__tests__/src/3-runtime-html/repeat.batched.spec.ts b/packages/__tests__/src/3-runtime-html/repeat.batched.spec.ts new file mode 100644 index 0000000000..4a1c8b28aa --- /dev/null +++ b/packages/__tests__/src/3-runtime-html/repeat.batched.spec.ts @@ -0,0 +1,108 @@ +import { createFixture } from "@aurelia/testing"; + +describe("3-runtime-html/repeat.batched.spec.ts", function () { + describe('tests that failed before batched array mutation fixes', function () { + it('combined remove and sort', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [1, 2, 3, 4]; } + ); + assertText('1234'); + component.items = [4, 1]; + assertText('41'); + }); + }); + + describe('additional tests', function () { + it('sort descending and remove [2, 3]', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [1, 2, 3, 4]; } + ); + assertText('1234'); + component.items = [4, 1]; + assertText('41'); + }); + + it('sort descending and remove [2, 4, 5]', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [1, 2, 3, 4, 5]; } + ); + assertText('12345'); + component.items = [5, 3]; + assertText('53'); + }); + + it('sort ascending and add [1, 2]', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [3, 4, 5]; } + ); + assertText('345'); + component.items = [1, 5, 3, 2]; + assertText('1532'); + }); + + it('remove duplicates [3] and sort remaining', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [2, 2, 3, 3, 4, 4]; } + ); + assertText('223344'); + component.items = [4, 2]; + assertText('42'); + }); + + it('sort ascending and remove [5, 7]', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [7, 5, 9, 1]; } + ); + assertText('7591'); + component.items = [9, 1]; + assertText('91'); + }); + + it('shuffle without sort or remove', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [1, 2, 3, 4]; } + ); + assertText('1234'); + component.items = [3, 1, 4, 2]; + assertText('3142'); + }); + + it('sort and replace [4, 5] with [2, 3]', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [4, 5, 6]; } + ); + assertText('456'); + component.items = [2, 3, 6]; + assertText('236'); + }); + + it('sort ascending and remove [4, 5]', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [5, 4, 3, 2, 1]; } + ); + assertText('54321'); + component.items = [1, 2, 3]; + assertText('123'); + }); + + it('complete replacement with [1, 2, 3]', function () { + const { assertText, component } = createFixture( + `
\${i}
`, + class { items = [8, 9, 10]; } + ); + assertText('8910'); + component.items = [1, 2, 3]; + assertText('123'); + }); + + }); +}); diff --git a/packages/__tests__/src/3-runtime-html/repeat.duplicates.spec.ts b/packages/__tests__/src/3-runtime-html/repeat.duplicates.spec.ts new file mode 100644 index 0000000000..7e7af0f42b --- /dev/null +++ b/packages/__tests__/src/3-runtime-html/repeat.duplicates.spec.ts @@ -0,0 +1,94 @@ +import { createFixture } from "@aurelia/testing"; + +describe("3-runtime-html/repeat.duplicates.spec.ts", function () { + describe('yield correct $index', function () { + it('duplicate primitive string', function () { + const { assertText, component } = createFixture( + `
\${$index}-\${i}
`, + class { items = ['a', 'b', 'a']; } + ); + assertText('0-a 1-b 2-a '); + + component.items.push('a'); + + assertText('0-a 1-b 2-a 3-a '); + }); + + it.skip('duplicate primitive string + push + sort', function () { + const { assertText, component } = createFixture( + `
\${$index}-\${i}
`, + class { items = ['a', 'b', 'a']; } + ); + assertText('0-a 1-b 2-a '); + + component.items.sort(); + assertText('0-a 1-a 2-b '); + }); + + it('duplicate primitive number', function () { + const { assertText, component } = createFixture( + `
\${$index}-\${i}
`, + class { items = [0, 1, 0]; } + ); + assertText('0-0 1-1 2-0 '); + + component.items.push(0); + + assertText('0-0 1-1 2-0 3-0 '); + }); + + it.skip('duplicate primitive number + sort', function () { + const { assertText, component } = createFixture( + `
\${$index}-\${i}
`, + class { items = [0, 1, 0]; } + ); + assertText('0-0 1-1 2-0 '); + + component.items.sort(); + + assertText('0-0 1-0 2-0 3-1 '); + }); + + it('duplicate object', function () { + const obj0 = { toString() { return '0'; } }; + const obj1 = { toString() { return '1'; } }; + + const { assertText, component } = createFixture( + `
\${$index}-\${i}
`, + class { items = [obj0, obj1, obj0]; } + ); + assertText('0-0 1-1 2-0 '); + + component.items.push(obj0); + + assertText('0-0 1-1 2-0 3-0 '); + }); + + it.skip('duplicate object + sort', function () { + const obj0 = { toString() { return '0'; } }; + const obj1 = { toString() { return '1'; } }; + + const { assertText, component } = createFixture( + `
\${$index}-\${i}
`, + class { items = [obj0, obj1, obj0]; } + ); + assertText('0-0 1-1 2-0 '); + + component.items.sort(); + + assertText('0-0 1-0 2-1 '); + }); + + // TODO: fix contextual props $index when sorting + it.skip('primitive string + sort (move to contextual props tests)', function () { + const { assertText, component } = createFixture( + `
\${$index}-\${i}
`, + class { items = ['c', 'b', 'a']; } + ); + assertText('0-c 1-b 2-a '); + + component.items.sort(); + assertText('0-a 1-b 2-c '); + }); + }); +}); diff --git a/packages/aurelia/src/index.ts b/packages/aurelia/src/index.ts index b55ae4e91b..46d89d7ad2 100644 --- a/packages/aurelia/src/index.ts +++ b/packages/aurelia/src/index.ts @@ -306,8 +306,6 @@ export { // ArrayObserver, // enableArrayObservation, // disableArrayObservation, - // applyMutationsToIndices, - // synchronizeIndices, // MapObserver, // enableMapObservation, diff --git a/packages/runtime-html/src/resources/template-controllers/repeat.ts b/packages/runtime-html/src/resources/template-controllers/repeat.ts index 84cec861e2..2a407430cd 100644 --- a/packages/runtime-html/src/resources/template-controllers/repeat.ts +++ b/packages/runtime-html/src/resources/template-controllers/repeat.ts @@ -1,6 +1,5 @@ import { type IDisposable, onResolve, IIndexable } from '@aurelia/kernel'; import { - applyMutationsToIndices, BindingBehaviorExpression, BindingContext, type Collection, @@ -13,7 +12,6 @@ import { type IOverrideContext, type IsBindingBehavior, Scope, - synchronizeIndices, ValueConverterExpression, astEvaluate, astAssign, @@ -48,6 +46,7 @@ export class Repeat implements ICustomAttribut /** @internal */ protected static inject = [IInstruction, IExpressionParser, IRenderLocation, IController, IViewFactory]; public views: ISyntheticView[] = []; + private _oldViews: ISyntheticView[] = []; public forOf!: ForOfStatement; public local!: string; @@ -190,6 +189,7 @@ export class Repeat implements ICustomAttribut /** @internal */ private _applyIndexMap(collection: Collection, indexMap: IndexMap | undefined): void { const oldViews = this.views; + this._oldViews = oldViews.slice(); const oldLen = oldViews.length; const key = this.key; const hasKey = key !== null; @@ -364,21 +364,20 @@ export class Repeat implements ICustomAttribut ); if (isPromise(ret)) { ret.catch(rethrow); } } else { - const $indexMap = applyMutationsToIndices(indexMap); // first detach+unbind+(remove from array) the deleted view indices - if ($indexMap.deletedIndices.length > 0) { + if (indexMap.deletedIndices.length > 0) { const ret = onResolve( - this._deactivateAndRemoveViewsByKey($indexMap), + this._deactivateAndRemoveViewsByKey(indexMap), () => { // TODO(fkleuver): add logic to the controller that ensures correct handling of race conditions and add a variety of `if` integration tests - return this._createAndActivateAndSortViewsByKey(oldLen, $indexMap); + return this._createAndActivateAndSortViewsByKey(oldLen, indexMap!); }, ); if (isPromise(ret)) { ret.catch(rethrow); } } else { // TODO(fkleuver): add logic to the controller that ensures correct handling of race conditions and add integration tests // eslint-disable-next-line @typescript-eslint/no-floating-promises - this._createAndActivateAndSortViewsByKey(oldLen, $indexMap); + this._createAndActivateAndSortViewsByKey(oldLen, indexMap); } } } @@ -496,7 +495,7 @@ export class Repeat implements ICustomAttribut const { $controller, views } = this; - const deleted = indexMap.deletedIndices; + const deleted = indexMap.deletedIndices.slice().sort(compareNumber); const deletedLen = deleted.length; let i = 0; for (; deletedLen > i; ++i) { @@ -509,10 +508,8 @@ export class Repeat implements ICustomAttribut } i = 0; - let j = 0; for (; deletedLen > i; ++i) { - j = deleted[i] - i; - views.splice(j, 1); + views.splice(deleted[i] - i, 1); } if (promises !== void 0) { @@ -533,7 +530,7 @@ export class Repeat implements ICustomAttribut let viewScope: Scope; let i = 0; - const { $controller, _factory, local, _normalizedItems, _location, views, _hasDestructuredLocal, _forOfBinding, _scopeMap, forOf } = this; + const { $controller, _factory, local, _normalizedItems, _location, views, _hasDestructuredLocal, _forOfBinding, _scopeMap, _oldViews, forOf } = this; const mapLen = indexMap.length; for (; mapLen > i; ++i) { @@ -549,7 +546,13 @@ export class Repeat implements ICustomAttribut const parentScope = $controller.scope; const newLen = indexMap.length; - synchronizeIndices(views, indexMap); + let source = 0; + i = 0; + for (; i < indexMap.length; ++i) { + if ((source = indexMap[i]) !== -2) { + views[i] = _oldViews[source]; + } + } // this algorithm retrieves the indices of the longest increasing subsequence of items in the repeater // the items on those indices are not moved; this minimizes the number of DOM operations that need to be performed @@ -833,3 +836,5 @@ const ensureUnique = (item: T, index: number): T | string => { return item; } }; + +const compareNumber = (a: number, b: number): number => a - b; diff --git a/packages/runtime/src/index.ts b/packages/runtime/src/index.ts index 7394c3b57c..6fe5494272 100644 --- a/packages/runtime/src/index.ts +++ b/packages/runtime/src/index.ts @@ -78,8 +78,6 @@ export { ArrayIndexObserver, enableArrayObservation, disableArrayObservation, - applyMutationsToIndices, - synchronizeIndices, type IArrayIndexObserver, } from './observation/array-observer'; export { diff --git a/packages/runtime/src/observation/array-observer.ts b/packages/runtime/src/observation/array-observer.ts index 52089b5296..b202882fd0 100644 --- a/packages/runtime/src/observation/array-observer.ts +++ b/packages/runtime/src/observation/array-observer.ts @@ -3,7 +3,6 @@ import { AccessorType, type ISubscriberCollection, type ICollectionSubscriberCollection, - cloneIndexMap, type IObserver, type CollectionKind, type ICollectionObserver, @@ -344,7 +343,7 @@ const observe = { break; } } - if (shouldNotify) { + if (shouldNotify || batching) { o.notify(); } return this; @@ -508,62 +507,3 @@ export function getArrayObserver(array: unknown[]): ArrayObserver { } return observer; } - -/** - * A compare function to pass to `Array.prototype.sort` for sorting numbers. - * This is needed for numeric sort, since the default sorts them as strings. - */ -const compareNumber = (a: number, b: number): number => a - b; - -/** - * Applies offsets to the non-negative indices in the IndexMap - * based on added and deleted items relative to those indices. - * - * e.g. turn `[-2, 0, 1]` into `[-2, 1, 2]`, allowing the values at the indices to be - * used for sorting/reordering items if needed - */ -export function applyMutationsToIndices(indexMap: IndexMap): IndexMap { - let offset = 0; - let j = 0; - let i = 0; - const $indexMap = cloneIndexMap(indexMap); - - // during a batch, items could be deleted in a non-linear order with multiple splices - if ($indexMap.deletedIndices.length > 1) { - // TODO: also synchronize deletedItems when we need them - $indexMap.deletedIndices.sort(compareNumber); - } - - const len = $indexMap.length; - for (; i < len; ++i) { - while ($indexMap.deletedIndices[j] <= i - offset) { - ++j; - --offset; - } - if ($indexMap[i] === -2) { - ++offset; - } else { - $indexMap[i] += offset; - } - } - return $indexMap; -} - -/** - * After `applyMutationsToIndices`, this function can be used to reorder items in a derived - * array (e.g. the items in the `views` in the repeater are derived from the `items` property) - */ -export function synchronizeIndices(items: T[], indexMap: IndexMap): void { - const copy = items.slice(); - - const len = indexMap.length; - let to = 0; - let from = 0; - while (to < len) { - from = indexMap[to]; - if (from !== -2) { - items[to] = copy[from]; - } - ++to; - } -} diff --git a/packages/runtime/src/observation/subscriber-batch.ts b/packages/runtime/src/observation/subscriber-batch.ts index a5a9cd19e6..0a7e383198 100644 --- a/packages/runtime/src/observation/subscriber-batch.ts +++ b/packages/runtime/src/observation/subscriber-batch.ts @@ -81,6 +81,8 @@ export function addCollectionBatch( ) { if (!currBatch!.has(subs)) { currBatch!.set(subs, [2, collection, indexMap]); + } else { + currBatch!.get(subs)![2] = indexMap; } }