Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions packages/@glimmer-workspace/integration-tests/lib/render-test.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated cleanup for easier debugging with breakpoints

Original file line number Diff line number Diff line change
Expand Up @@ -448,14 +448,16 @@ export class RenderTest implements IRenderTest {
private assertEachCompareResults(
items: (number | string | [string | number, string | number])[]
) {
[...(this.element as unknown as HTMLElement).querySelectorAll('.test-item')].forEach(
(el, index) => {
let key = Array.isArray(items[index]) ? items[index][0] : index;
let value = Array.isArray(items[index]) ? items[index][1] : items[index];
let rendered = (this.element as unknown as HTMLElement).querySelectorAll('.test-item');

QUnit.assert.equal(el.textContent, `${key}.${value}`, `Comparing the rendered key.value`);
}
);
QUnit.assert.strictEqual(rendered.length, items.length, `Comparing the rendered item count`);

rendered.forEach((el, index) => {
let key = Array.isArray(items[index]) ? items[index][0] : index;
let value = Array.isArray(items[index]) ? items[index][1] : items[index];

QUnit.assert.equal(el.textContent, `${key}.${value}`, `Comparing the rendered key.value`);
});
}

protected assertReactivity<T>(
Expand Down
40 changes: 40 additions & 0 deletions packages/@glimmer-workspace/integration-tests/lib/suites/each.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests preserve accidentally existing behavior (found by @johanrd )

Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,46 @@ export class EachSuite extends RenderTest {
this.assertStableNodes();
}

@test
'a plain array that shrinks during iteration stops the iteration'() {
let list: Item[] = [];

// The getter is on the prototype so that `JSON.stringify` in `render` does not call it.
class Item {
constructor(private index: number) {}

get name() {
if (this.index === 0) list.length = 1;
return `item-${this.index}`;
}
}

for (let i = 0; i < 4; i++) list.push(new Item(i));

this.render('{{#each this.list key="@index" as |item|}}[{{item.name}}]{{/each}}', { list });
this.assertHTML('[item-0]');
}

@test
'a plain array that grows during iteration renders the new items'() {
let list: Item[] = [];

// The getter is on the prototype so that `JSON.stringify` in `render` does not call it.
class Item {
constructor(private index: number) {}

get name() {
if (this.index === 0) list.push(new Item(1));
return `item-${this.index}`;
}
}

list.push(new Item(0));

this.render('{{#each this.list key="@index" as |item|}}[{{item.name}}]{{/each}}', { list });
this.assertHTML('[item-0][item-1]');
}

@test
'autotracked custom iterable'() {
if (typeof Symbol !== 'function') {
Expand Down
33 changes: 10 additions & 23 deletions packages/@glimmer/reference/lib/iterable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,42 +225,29 @@ class IteratorWrapper implements OpaqueIterator {
}

class ArrayIterator implements OpaqueIterator {
private current: { kind: 'empty' } | { kind: 'first'; value: unknown } | { kind: 'progress' };
private pos = 0;
private pos = -1;
private empty: boolean;

constructor(
private iterator: unknown[] | readonly unknown[],
private keyFor: KeyFor
) {
if (iterator.length === 0) {
this.current = { kind: 'empty' };
} else {
this.current = { kind: 'first', value: iterator[this.pos] };
}
// Only this read runs in the tracking frame of the iterator reference.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is kinda "womp"

this.empty = iterator.length === 0;
}

isEmpty(): boolean {
return this.current.kind === 'empty';
return this.empty;
}

next(): Nullable<IterationItem<unknown, number>> {
let value: unknown;

let current = this.current;
if (current.kind === 'first') {
this.current = { kind: 'progress' };
value = current.value;
} else if (this.pos >= this.iterator.length - 1) {
return null;
} else {
value = this.iterator[++this.pos];
}
let memo = ++this.pos;

let { keyFor } = this;
// The length is live because code in the block can change a plain array.
if (memo >= this.iterator.length) return null;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this implicit behavior we had here (and continue to have here): entangling with length, if it's tracked, could be why some of our iteration/updates is slow.

I need to think about how we can have a better list update/create/destroy algo in general, but that'll be a much bigger PR


let key = keyFor(value, this.pos);
let memo = this.pos;
let value = this.iterator[memo];

return { key, value, memo };
return { key: this.keyFor(value, memo), value, memo };
}
}
Loading