Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rerender an {{#eachs block only when the specific item has changed #1100

Merged
merged 2 commits into from
May 15, 2020
Merged

Rerender an {{#eachs block only when the specific item has changed #1100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce0620c
[BUGFIX] Update iterable items only when changed
May 15, 2020
c4215f4
add todo
May 15, 2020
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
63 changes: 61 additions & 2 deletions packages/@glimmer/integration-tests/lib/suites/each.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { RenderTest } from '../render-test';
import { test } from '../test-decorator';
import { tracked } from '../test-helpers/tracked';

export class EachSuite extends RenderTest {
static suiteName = '#each';
Expand Down Expand Up @@ -165,6 +166,38 @@ export class EachSuite extends RenderTest {
'it renders all items with duplicate key values'() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was not testing what it says it was, so I updated it to test that properly (which it does) and added a couple more tests

let list = [{ text: 'Hello' }, { text: 'Hello' }, { text: 'Hello' }];

this.render(`{{#each list key="text" as |item|}}{{item.text}}{{/each}}`, {
list,
});

this.assertHTML('HelloHelloHello');
this.assertStableRerender();

list.forEach(item => (item.text = 'Goodbye'));

this.rerender({ list });
this.assertHTML('GoodbyeGoodbyeGoodbye');
this.assertStableNodes();

list = [{ text: 'Hello' }, { text: 'Hello' }, { text: 'Hello' }];

this.rerender({ list });
this.assertHTML('HelloHelloHello');
this.assertStableNodes();
}

@test
'it updates items if their key has not changed, and the items are tracked'() {
class Item {
@tracked public text: string;

constructor(text: string) {
this.text = text;
}
}

let list = [new Item('Hello'), new Item('Hello'), new Item('Hello')];

this.render(`{{#each list key="@identity" as |item|}}{{item.text}}{{/each}}`, {
list,
});
Expand All @@ -185,6 +218,24 @@ export class EachSuite extends RenderTest {
this.assertStableNodes();
}

@test
'it does not update items if their key has not changed, and the items are not tracked'() {
let list = [{ text: 'Hello' }, { text: 'Hello' }, { text: 'Hello' }];

this.render(`{{#each list key="@identity" as |item|}}{{item.text}}{{/each}}`, {
list,
});

this.assertHTML('HelloHelloHello');
this.assertStableRerender();

list.forEach(item => (item.text = 'Goodbye'));

this.rerender({ list });
this.assertHTML('HelloHelloHello');
this.assertStableNodes();
}

@test
'scoped variable not available outside list'() {
let list = ['Wycats'];
Expand Down Expand Up @@ -243,6 +294,14 @@ export class EachSuite extends RenderTest {
}
}

function val(i: number): { val: number } {
return { val: i };
class Val {
@tracked val: number;

constructor(val: number) {
this.val = val;
}
}

function val(i: number): Val {
return new Val(i);
}
21 changes: 15 additions & 6 deletions packages/@glimmer/integration-tests/lib/suites/in-element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { equalsElement } from '../dom/assertions';
import { stripTight } from '../test-helpers/strings';
import { replaceHTML } from '../dom/simple-utils';
import { EmberishCurlyComponent } from '../components/emberish-curly';
import { tracked } from '../test-helpers/tracked';

export class InElementSuite extends RenderTest {
static suiteName = '#in-element';
Expand Down Expand Up @@ -388,20 +389,28 @@ export class InElementSuite extends RenderTest {

@test
'Inside a loop'() {
let { delegate } = this;

class Item {
element = delegate.createElement('div');

@tracked value: string;

constructor(value: string) {
this.value = value;
}
}

this.testType = 'Dynamic';
this.registerComponent('Basic', 'FooBar', '<p>{{@value}}</p>');

this.registerHelper('log', ([item]) => console.log(item));

let roots = [
{ id: 0, element: this.delegate.createElement('div'), value: 'foo' },
{ id: 1, element: this.delegate.createElement('div'), value: 'bar' },
{ id: 2, element: this.delegate.createElement('div'), value: 'baz' },
];
let roots = [new Item('foo'), new Item('bar'), new Item('baz')];

this.render(
stripTight`
{{~#each this.roots key="id" as |root|~}}
{{~#each this.roots as |root|~}}
{{~log root~}}
{{~#in-element root.element ~}}
<FooBar @value={{root.value}} />
Expand Down
15 changes: 15 additions & 0 deletions packages/@glimmer/integration-tests/lib/test-helpers/tracked.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { trackedData } from '@glimmer/validator';

export function tracked(target: object, key: string) {
let { getter, setter } = trackedData<any, any>(key);

Object.defineProperty(target, key, {
get() {
return getter(this);
},

set(value: unknown) {
setter(this, value);
},
});
}
9 changes: 7 additions & 2 deletions packages/@glimmer/reference/lib/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,13 @@ export class IterationItemReference<T = unknown> implements TemplatePathReferenc
}

update(value: T) {
dirtyTag(this.tag);
this.itemValue = value;
let { itemValue } = this;

// TODO: refactor this https://github.com/glimmerjs/glimmer-vm/issues/1101
if (value !== itemValue) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind me what itemValue actually represents? Is it the value that we find after looking up the key path (or guidFor if key="@identity")? Or is it literally the item being yielded?

Copy link

Choose a reason for hiding this comment

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

Or is it literally the item being yielded?

This. If the "iterable" is an array, then for each value in the array you have an IterationItemReference where itemValue is the value.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t it be the key that we are checking for stability (not the item itself)? I realize that in the default case for Ember there is no difference between the two (since Ember uses key=“@identity” by default), but it is possible to specify a key and AFAICT that is the thing that is important to check for stability 🤔.

For example, imagine the case where the array and its items are always regenerated (e.g. the immutable pattern) so you’ve used key to tell {{#each that it should care about some stable property on the item (e.g. id).

Copy link

@sandydoo sandydoo May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICT we're not checking the stability here. The key is used when checking the stability of the array itself via keyFor — checking which items were added, which were moved, and which stayed in the same place. If the array is touched and an item is considered retained (by comparing keys), then it just always updates the value and the memo (in the case of arrays, the array index). If you don't compare the values at this point, doing anything to the array can become quite an expensive operation O(n).

There's more though.

If you have an array of objects and modify some property deep inside, then all of this iterator code is skipped and just that nested PropertyReference is updated. So the key you provide to {{each}} plays no role in that case. But doing anything to array will trigger the iterator synchronization/list revalidation code and call this update method. So we do all this extra work, even though in some cases Ember provides the exact key modified via dirtyTagFor.

Also, {{each-in}} is possibly even more broken. Since the default key is @identity, updating the value of an existing key won't cause an update, as may be expected by an end user. It will actually destroy and recreate the item. So, again, if you're using a render modifier, you'll get a bunch of weird triggers.

TBH I don't understand why we should be comparing values here at all. Aren't there tags for every value anyways? Shouldn't we be checking those instead?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, glad you dug in here 😄.

@pzuraq - Any reason we are checking === vs a tag validation?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the opposite in this case - we know that the value has possibly changed, and we need to dirty the tag if so and update it. We don't have another tag to check.

dirtyTag(this.tag);
this.itemValue = value;
}
}

get(key: string): TemplatePathReference {
Expand Down