Skip to content

Commit

Permalink
fix(repeater): duplicate primitive handling, batched mutation fix (#1840
Browse files Browse the repository at this point in the history 
)

* fix(repeat): fix sort+splice batched operation bug

* test(repeat): verify $index duplicate bug, add skipped test for contextual props mutation bug
  • Loading branch information
@fkleuver
fkleuver authored Dec 5, 2023
1 parent 342847f commit 703d275
Show file tree
Hide file tree
Showing 8 changed files with 408 additions and 91 deletions.
198 changes: 185 additions & 13 deletions packages/__tests__/src/2-runtime/array-observer.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import {
enableArrayObservation,
ICollectionSubscriber,
IndexMap,
applyMutationsToIndices,
synchronizeIndices,
batch,
Collection,
} from '@aurelia/runtime';
Expand All @@ -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[];
Expand All @@ -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 () {
Expand Down Expand Up @@ -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 () {
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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();
});
});
});
});
});

Expand Down
108 changes: 108 additions & 0 deletions packages/__tests__/src/3-runtime-html/repeat.batched.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
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(
`<div repeat.for="i of items">\${i}</div>`,
class { items = [8, 9, 10]; }
);
assertText('8910');
component.items = [1, 2, 3];
assertText('123');
});

});
});
Loading

0 comments on commit 703d275

Please sign in to comment.