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
#each re-render bug #16975
Comments
@andrewtimberlake seems like using the But does seem like a bug, the |
I think I know what's going on here. It's a 🍝 of mess but I'll try to describe it. A lot of edge cases (border line user-error) contributed to this, and I'm not really sure what is/is not a bug, what and how to fix any of these. Major 🔑First of all, I need to describe what the For our purpose, let's accept as a given that "touching DOM" (e.g. updating the content of a text node, an attribute, adding or removing content etc) is expensive and is to be avoided as much as possible. Let's focus on a rather simple piece of template: <ul>
{{#each this.names as |name|}}
<li>{{name.first}} {{to-upper-case name.last}}</li>
{{/each}}
</ul> If [
{ first: "Yehuda", last: "Katz" },
{ first: "Tom", last: "Dale" },
{ first: "Godfrey", last: "Chan" }
] Then you will get... <ul>
<li>Yehuda KATZ</li>
<li>Tom DALE</li>
<li>Godfrey CHAN</li>
</ul> So far so good. Appending an item to the listNow what if we append <ul>
<li>Yehuda KATZ</li>
<li>Tom DALE</li>
<li>Godfrey CHAN</li>
<li>Andrew TIMBERLAKE</li>
</ul> But how? The most naive way to implement the
This seems... very unnecessary and expensive. We know the first three items didn't change, so it would be nice if we could just skip the work for those rows. 🔑 @IndexA better implementation would be to try to reuse the existing rows and don't do any unnecessary updates. One idea would be to simply match up the rows with their positions in the templates. This is essentially what
So, with this implementation, we reduced the total number of operations from 23 to 5 (👋 hand-waving over the cost of the comparisons, but for our purpose, we are assuming they are relatively cheap compared to the rest). Not bad. Prepending an item to the listBut now, what would happen if, instead of appending <ul>
<li>Andrew TIMBERLAKE</li>
<li>Yehuda KATZ</li>
<li>Tom DALE</li>
<li>Godfrey CHAN</li>
</ul> But how?
That's 14 operations. Ouch! 🔑 @identityThat seemed unnecessary, because conceptually, whether we are prepending or appending, we are still only changing (inserting) a single object in the array. Optimally, we should be able to handle this case just as well as we did in the append scenario. This is where
With that, we are back to the optimal 5 operations. Scaling UpAgain this is 👋 hand-waving over the comparisons and book-keeping costs. Indeed, those are not free either, and in this very simple example, they may not be worth it. But imagine the list is big and each row invokes a complicated component (with lots of helpers, computed properties, sub-components, etc). Imagine the LinkedIn news-feed, for example. If we don't match up the right rows with the right data, your components' arguments can potentially churn a lot and causes much more DOM updates than you may otherwise expect. There are also issues with matching up the wrong DOM elements and loosing DOM state, such as cursor position and text selection state. Overall, the extra comparison and book-keeping cost is easily worth most of the time in a real-world app. Since the Collisions 💥But wait, there is a problem. What about this case? const YEHUDA = { first: "Yehuda", last: "Katz" };
const TOM = { first: "Tom", last: "Dale" };
const GODFREY = { first: "Godfrey", last: "Chan" };
this.list = [
YEHUDA,
TOM,
GODFREY,
TOM, // duplicate
YEHUDA, // duplicate
YEHUDA, // duplicate
YEHUDA // duplicate
]; The problem here is that the same object could appear multiple times in the same list. This breaks our naive To avoid this, we use sort of a hybrid approach to handle these collisions. Internally, the keys-to-DOM mapping looks something like this:
For the most part, this is pretty rare, and when it does come up, this works Good Enough™ most of the time. If, for some reason, this doesn't work, you can always use a key path (or the even more advanced keying mechanism in RFC 321). Back to the "🐛"After all that talk we are now ready to look at the scenario in the Twiddle. Essentially, we started with this list:
Since we didn't specify the key, Ember uses
Now, let's say we click the first check box:
How is the DOM updated?
So this explains how we got the output you had in the twiddle. Essentially all the DOM rows shifted down by one, and a new one was inserted at the top, while the The really unexpected part is 2.2. Even though the first checkbox (the one that was clicked on) was shifted down by one row to the second position, you would probably have expected Ember to its But this is not how it works. As mentioned in the beginning, accessing DOM is expensive. This includes reading from DOM. If, on every update, we had to read the latest value from the DOM for our comparisons, it would pretty much defeat the purpose of our optimizations. Therefore, in order to avoid that, we remembered the last value that we had written to the DOM, and compare the current value against the cached value without having to read it back from the DOM. Only when there is a difference do we write the new value to the DOM (and cache it for next time). This is the sense in which we kind of share the same "virtual DOM" approach, but we only do it at the leaf nodes, not virtualizing the "tree-ness" of the whole DOM. So, TL;DR, "binding" the This is why the I'm not sure where that leaves us. I understand why this is surprising, but I'm inclined to say this is a series of unfortunate user errors. Perhaps we should be linting against binding these properties on input elements? |
Relevant code: ember.js/packages/@ember/-internals/glimmer/lib/utils/iterable.ts Lines 390 to 391 in c24bc23
ember.js/packages/@ember/-internals/glimmer/lib/utils/iterable.ts Lines 436 to 445 in c24bc23
ember.js/packages/@ember/-internals/glimmer/lib/utils/iterable.ts Lines 451 to 466 in c24bc23
|
@chancancode - thanks for the amazing explanation. Does that mean that |
@boris-petrov there may be some limited cases where it is acceptable.. like a readonly textfield for "copy this url to your clipboard" kind of thing, or you can use the input element + However, that still wouldn't have "fixed" this case where there are collisions with the keys. See https://ember-twiddle.com/0f2369021128e2ae0c445155df5bb034?openFiles=templates.application.hbs%2C Which is why I said, I'm 100% not sure what to do about this. On one hand I agree it is surprising and unexpected, on the other hand, this kind of collision is pretty rare in real apps and it is why the "key" argument is customizable (this is a case where the default "@identity" key function is not Good Enough™, which is why that feature exists). |
@chancancode - this reminds me of another issue that I opened some time ago. Do you think there is something similar there? The answer that I received there (about the need to use |
@boris-petrov I don't think it is related |
hi, we use sortablejs for draggable list with ember . pls check this demo for reproduce each problem . step:
you can see the dragged item stay in dom tree . but, if drag item to other position(not last item) , it seems that work well . |
I have an array of true/false/undefined values which I am rendering as a list of checkboxes.
When changing an array element to or from true, the list of checkboxes is re-rendered with the following (index+1) checkbox inheriting the change along with the changed checkbox.
Code:
When I use
{{#each range key="@index" as |value idx|}}
it works correctly.Twiddle: https://ember-twiddle.com/6d63548f35f99da19cee9f58fb64db59
The text was updated successfully, but these errors were encountered: