-
Notifications
You must be signed in to change notification settings - Fork 192
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.
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 {{#each
s block only when the specific item has changed
#1100
Conversation
Currently, iterable item references always update their value and dirty their tag, even if the value didn't actually change. This is different from the baseline behavior in Ember, and a regression. This PR updates this behavior, and also updates affected tests. These tests were written prior to the recent VM upgrade, and probably assumed some differences in behaviors in the testing environment. Now that we're trying to align the testing environment more closely with Ember, these tests need to be updated. I added a `@tracked` decorator to the test helpers, which should allow us to more closely match Ember's behavior.
7b6266a
to
ce0620c
Compare
@@ -165,6 +166,38 @@ export class EachSuite extends RenderTest { | |||
'it renders all items with duplicate key values'() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Does each-in
also exist in this repo? IIRC the issue report in Ember suggested that it had the same problem, if it lives here would you mind adding a test ensuring stability for each-in
as well?
this.itemValue = value; | ||
let { itemValue } = this; | ||
|
||
if (value !== itemValue) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
each-in does not exist in Glimmer VM, it is a template transform to each provided by Ember. We should test it in Ember |
{{#each
s block only when the specific item has changed
Currently, iterable item references always update their value and dirty
their tag, even if the value didn't actually change. This is different
from the baseline behavior in Ember, and a regression.
This PR updates this behavior, and also updates affected tests. These
tests were written prior to the recent VM upgrade, and probably assumed
some differences in behaviors in the testing environment. Now that we're
trying to align the testing environment more closely with Ember, these
tests need to be updated. I added a
@tracked
decorator to the testhelpers, which should allow us to more closely match Ember's behavior.
Fix for emberjs/ember.js#18968