Skip to content

Commit

Permalink
fix(repeat): fix mismatchedLengthError on assigning an array with dup…
Browse files Browse the repository at this point in the history 
…licate primitive values (#1737)
  • Loading branch information
@fkleuver
fkleuver committed Apr 11, 2023
1 parent bd18fde commit cf60ac8
Show file tree
Hide file tree
Showing 2 changed files with 233 additions and 29 deletions.
187 changes: 186 additions & 1 deletion packages/__tests__/3-runtime-html/repeat.keyed.array.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ describe("3-runtime-html/repeat.keyed.array.spec.ts", function () {
component: ICustomElementViewModel & Component;
};

describe('non-keyed', function () {
describe('non-keyed with reference types', function () {
async function testFn(fn: (ctx: $ctx) => Promise<void>) {
const ctx = TestContext.create();
const au = new Aurelia(ctx.container);
Expand Down Expand Up @@ -170,6 +170,191 @@ describe("3-runtime-html/repeat.keyed.array.spec.ts", function () {
});
});

describe('non-keyed with value types', function () {
async function testFn(fn: (ctx: $ctx) => Promise<void>) {
const ctx = TestContext.create();
const au = new Aurelia(ctx.container);
const host = ctx.createElement("div");

const App = CustomElement.define(
{
name: "app",
template: `<div repeat.for="i of items">\${i}</div>`
},
Component,
);

const mutations: MutationRecord[] = [];
const obs = new ctx.wnd.MutationObserver(_mutations => mutations.splice(0, mutations.length, ..._mutations));

const component = new App();
au.app({ host, component });

async function mutate(cb: () => void) {
obs.observe(host, { childList: true });
cb();
await Promise.resolve();
obs.disconnect();
}

try {
await fn({ au, host, mutations, mutate, component });
} finally {
await au.stop();
au.dispose();
}
}

const $it = create$it(testFn);

$it('mutate: simple replacement', async function ({ au, host, mutations, mutate, component }) {
component.items = [0, 1, 2, 3, 4];

await au.start();
assert.strictEqual(host.textContent, '01234');

await mutate(() => {
component.items.splice(4, 1, 4);
});

assert.strictEqual(host.textContent, '01234');
assert.strictEqual(mutations.length, 2);
assertRem(0, mutations, 4);
assertAdd(1, mutations, 4);
});

$it('mutate: simple move', async function ({ au, host, mutations, mutate, component }) {
component.items = [0, 1, 2, 3, 4];

await au.start();
assert.strictEqual(host.textContent, '01234');

await mutate(() => {
batch(() => {
component.items.splice(4, 1, 0);
component.items.splice(0, 1, 4);
});
});

assert.strictEqual(host.textContent, '41230');
assert.strictEqual(mutations.length, 4);
assertRem(0, mutations, 0, 4);
assertAdd(2, mutations, 0, 4);
});

$it('reassign with duplicate numbers: shift to left', async function ({ au, host, mutations, mutate, component }) {
component.items = [0, 0, 1, 1];

await au.start();
assert.strictEqual(host.textContent, '0011');

await mutate(() => {
component.items = [0, 1, 1, 1];
});

assert.strictEqual(host.textContent, '0111');
assert.strictEqual(mutations.length, 2);
assertRem(0, mutations, 0);
assertAdd(1, mutations, 1);
});

$it('reassign with duplicate numbers: shift to right', async function ({ au, host, mutations, mutate, component }) {
component.items = [0, 0, 1, 1];

await au.start();
assert.strictEqual(host.textContent, '0011');

await mutate(() => {
component.items = [0, 0, 0, 1];
});

assert.strictEqual(host.textContent, '0001');
assert.strictEqual(mutations.length, 2);
assertRem(0, mutations, 1);
assertAdd(1, mutations, 0);
});

$it('reassign with duplicate strings: shift to left', async function ({ au, host, mutations, mutate, component }) {
component.items = ['0', '0', '1', '1'];

await au.start();
assert.strictEqual(host.textContent, '0011');

await mutate(() => {
component.items = ['0', '1', '1', '1'];
});

assert.strictEqual(host.textContent, '0111');
assert.strictEqual(mutations.length, 2);
assertRem(0, mutations, '0');
assertAdd(1, mutations, '1');
});

$it('reassign with duplicate booleans: shift to left', async function ({ au, host, mutations, mutate, component }) {
component.items = [false, false, true, true];

await au.start();
assert.strictEqual(host.textContent, 'falsefalsetruetrue');

await mutate(() => {
component.items = [false, true, true, true];
});

assert.strictEqual(host.textContent, 'falsetruetruetrue');
assert.strictEqual(mutations.length, 2);
assertRem(0, mutations, false);
assertAdd(1, mutations, true);
});

$it('reassign with duplicate null values: shift to left', async function ({ au, host, mutations, mutate, component }) {
component.items = [undefined, undefined, null, null];

await au.start();
assert.strictEqual(host.textContent, '');

await mutate(() => {
component.items = [undefined, null, null, null];
});

assert.strictEqual(host.textContent, '');
assert.strictEqual(mutations.length, 2);
assertRem(0, mutations);
assertAdd(1, mutations);
});

$it('reassign with duplicate undefined values: shift to left', async function ({ au, host, mutations, mutate, component }) {
component.items = [null, null, undefined, undefined];

await au.start();
assert.strictEqual(host.textContent, '');

await mutate(() => {
component.items = [null, undefined, undefined, undefined];
});

assert.strictEqual(host.textContent, '');
assert.strictEqual(mutations.length, 2);
assertRem(0, mutations);
assertAdd(1, mutations);
});

$it('reassign with duplicate strings and numbers: replace strings with numbers (ensure that non-strict-equal values are considered different by the repeater too)', async function ({ au, host, mutations, mutate, component }) {
component.items = [0, 0, 1, 1];

await au.start();
assert.strictEqual(host.textContent, '0011');

await mutate(() => {
component.items = ['0', '0', '1', '1'];
});

assert.strictEqual(host.textContent, '0011');
assert.strictEqual(mutations.length, 8);
assertRem(0, mutations, 0, 0, 1, 1);
assertAdd(4, mutations, 1, 1, 0, 0);
});
});

for (const spec of [
{ title: 'literal', expr: 'i of items; key: k', text: '${i.k}' },
{ title: 'expression', expr: 'i of items; key.bind: i.k', text: '${i.k}' }
Expand Down
75 changes: 47 additions & 28 deletions packages/runtime-html/src/resources/template-controllers/repeat.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ export class Repeat<C extends Collection = unknown[]> implements ICustomAttribut
@bindable public items: Items<C>;
public key: null | string | IsBindingBehavior = null;

/** @internal */ private readonly _keyMap: Map<IIndexable, unknown> = new Map();
/** @internal */ private readonly _scopeMap: Map<IIndexable, Scope> = new Map();
/** @internal */ private readonly _keyMap: Map<unknown, unknown> = new Map();
/** @internal */ private readonly _scopeMap: Map<unknown, Scope> = new Map();
/** @internal */ private _observer?: CollectionObserver = void 0;
/** @internal */ private _innerItems: Items<C> | null;
/** @internal */ private _forOfBinding!: PropertyBinding;
Expand Down Expand Up @@ -246,8 +246,8 @@ export class Repeat<C extends Collection = unknown[]> implements ICustomAttribut
}
}

let oldItem: IIndexable;
let newItem: IIndexable;
let oldItem: unknown;
let newItem: unknown;
let oldKey: unknown;
let newKey: unknown;
let j = 0;
Expand All @@ -266,14 +266,15 @@ export class Repeat<C extends Collection = unknown[]> implements ICustomAttribut
// views with same key at start
// eslint-disable-next-line no-constant-condition
while (true) {
oldItem = oldItems[i];
newItem = newItems[i];
oldKey = hasKey
? getKeyValue(keyMap, key, oldItem, getScope(scopeMap, oldItems[i], forOf, parentScope, binding, local, hasDestructuredLocal), binding)
: oldItem;
newKey = hasKey
? getKeyValue(keyMap, key, newItem, getScope(scopeMap, newItems[i], forOf, parentScope, binding, local, hasDestructuredLocal), binding)
: newItem;
if (hasKey) {
oldItem = oldItems[i];
newItem = newItems[i];
oldKey = getKeyValue(keyMap, key, oldItem, getScope(scopeMap, oldItem, forOf, parentScope, binding, local, hasDestructuredLocal), binding);
newKey = getKeyValue(keyMap, key, newItem, getScope(scopeMap, newItem, forOf, parentScope, binding, local, hasDestructuredLocal), binding);
} else {
oldItem = oldKey = ensureUnique(oldItems[i], i);
newItem = newKey = ensureUnique(newItems[i], i);
}
if (oldKey !== newKey) {
keyMap.set(oldItem, oldKey);
keyMap.set(newItem, newKey);
Expand All @@ -295,14 +296,15 @@ export class Repeat<C extends Collection = unknown[]> implements ICustomAttribut
j = newEnd;
// eslint-disable-next-line no-constant-condition
while (true) {
oldItem = oldItems[j];
newItem = newItems[j];
oldKey = hasKey
? getKeyValue(keyMap, key, oldItem, getScope(scopeMap, oldItem, forOf, parentScope, binding, local, hasDestructuredLocal), binding)
: oldItem;
newKey = hasKey
? getKeyValue(keyMap, key, newItem, getScope(scopeMap, newItem, forOf, parentScope, binding, local, hasDestructuredLocal), binding)
: newItem;
if (hasKey) {
oldItem = oldItems[j];
newItem = newItems[j];
oldKey = getKeyValue(keyMap, key, oldItem, getScope(scopeMap, oldItem, forOf, parentScope, binding, local, hasDestructuredLocal), binding);
newKey = getKeyValue(keyMap, key, newItem, getScope(scopeMap, newItem, forOf, parentScope, binding, local, hasDestructuredLocal), binding);
} else {
oldItem = oldKey = ensureUnique(oldItems[i], i);
newItem = newKey = ensureUnique(newItems[i], i);
}
if (oldKey !== newKey) {
keyMap.set(oldItem, oldKey);
keyMap.set(newItem, newKey);
Expand All @@ -321,7 +323,7 @@ export class Repeat<C extends Collection = unknown[]> implements ICustomAttribut
const newStart = i;

for (i = newStart; i <= newEnd; ++i) {
if (keyMap.has(newItem = newItems[i])) {
if (keyMap.has(newItem = hasKey ? newItems[i] : ensureUnique(newItems[i], i))) {
newKey = keyMap.get(newItem);
} else {
newKey = hasKey
Expand All @@ -333,7 +335,7 @@ export class Repeat<C extends Collection = unknown[]> implements ICustomAttribut
}

for (i = oldStart; i <= oldEnd; ++i) {
if (keyMap.has(oldItem = oldItems[i])) {
if (keyMap.has(oldItem = hasKey ? oldItems[i] : ensureUnique(oldItems[i], i))) {
oldKey = keyMap.get(oldItem);
} else {
oldKey = hasKey
Expand All @@ -351,7 +353,7 @@ export class Repeat<C extends Collection = unknown[]> implements ICustomAttribut
}

for (i = newStart; i <= newEnd; ++i) {
if (!oldIndices.has(keyMap.get(newItems[i]))) {
if (!oldIndices.has(keyMap.get(hasKey ? newItems[i] : ensureUnique(newItems[i], i)))) {
indexMap[i] = -2;
}
}
Expand Down Expand Up @@ -789,17 +791,17 @@ const $number = (result: number, func: (item: number, index: number, arr: number
};

const getKeyValue = (
keyMap: WeakMap<IIndexable, unknown>,
keyMap: Map<unknown, unknown>,
key: string | IsBindingBehavior,
item: IIndexable,
item: unknown,
scope: Scope,
binding: PropertyBinding,
) => {

let value = keyMap.get(item);
if (value === void 0) {
if (typeof key === 'string') {
value = item[key];
value = (item as IIndexable)[key];
} else {
value = astEvaluate(key, scope, binding, null);
}
Expand All @@ -809,8 +811,8 @@ const getKeyValue = (
};

const getScope = (
scopeMap: WeakMap<IIndexable, Scope>,
item: IIndexable,
scopeMap: Map<unknown, Scope>,
item: unknown,
forOf: ForOfStatement,
parentScope: Scope,
binding: PropertyBinding,
Expand All @@ -828,3 +830,20 @@ const getScope = (
}
return scope;
};

const ensureUnique = <T>(item: T, index: number): T | string => {
const type = typeof item;
switch (type) {
case 'object':
if (item !== null) return item;
// falls through
case 'string':
case 'number':
case 'bigint':
case 'undefined':
case 'boolean':
return `${index}${type}${item}`;
default:
return item;
}
};

0 comments on commit cf60ac8

Please sign in to comment.