Delete virtual element #358
Conversation
The radar has a domPool virtual element to save temporarily deleted nodes and reuse them when a new item is added. So if the item is deleted from the items list the item's node can still exist in the virtual dom so the item's component can be still updated by Ember. This behavior isn't working for computed fields on the ember models. Ember rejects the error when trying to access the computed property on the destroyed object if it hasn't been consumed before. This is possible if the items' component should render more fields after the update. To avoid re-rendering deleted items it's better to delete related components from the virtual pool. It doesn't give much impact on performance as this can happen only if the component has fewer total items than virtual items after the update and it also doesn't affect scroll rendering.
| if (item) { | ||
| insertRangeBefore(this._domPool, null, component.realUpperBound, component.realLowerBound); | ||
| } else { | ||
| run(() => { |
There was a problem hiding this comment.
Deleting all virtual elements in the same runloop gives unexpected results in Ember >= 3.20. Please write if you need some extra details
There was a problem hiding this comment.
removeObject should be in a runloop 👍
There was a problem hiding this comment.
Some details on why it's needed its own runloop here.
The virtualComponents are out of order. See the description of _updateVirtualComponents to have more details.
Imagine you have 10 visible components: 11, 12, 13...20. (First 10 were scrolled down). As the method says they can be shuffled in {{each}} order to have the correct order in the dom (because of _appendComponent and _prependComponent). So the {{each}} order can be like this: 9, 8, 7, 4, 6, 0, 1, 2, 3, 5 (*1). So far so good.
And here the items array is updated so it has only 5 items. The method is trying to update the first 5 components and according to the {{each}} order (see *1) the content is set like this 9->0, 8->1, 7->2, 4->3, 6->4 (*2) and the other components should be deleted (it's what I want to reach to avoid updating deleted items).
And here looks like Ember>=3.20 has some optimization, it sees that items with primary order (0, 1, 2, 3, 5) (see *1 and *2) are deleted so he thinks it's better to render everything again with default order (4, 6, 7, 8, 9) (the ordered remaining items from the note *2). But the content of these items is (3, 4, 2, 1, 0). And you can see this in failing tests if you remove the run call here.
So having the own run call here says to the Ember to delete the items one by one so he doesn't trigger the optimization. Looks weird but the code is calling in the same JS runloop so wrapping into the run doesn't break any js logic there
| if (item) { | ||
| insertRangeBefore(this._domPool, null, component.realUpperBound, component.realLowerBound); | ||
| } else { | ||
| run(() => { |
There was a problem hiding this comment.
removeObject should be in a runloop 👍
| run(() => { | ||
| virtualComponents.removeObject(component); | ||
| }); | ||
| _componentPool.splice(i, 1); |
There was a problem hiding this comment.
Ah, the reverse traversal is so this mutation is safe, yes?
The insertRangeBefore( logic doesn't care about the order of traversal? I take it the answer is no, but I'm not so familiar with the code that I want to presume it without diving in.
There was a problem hiding this comment.
yep, as the indexes are only decreasing you are sure you don't miss any item of the _componentPool in the loop.
There is no sense in order in insertRangeBefore as the _componentPool is shuffled anyway
Add test to check there are no updates for the deleted nodes
|
@atrusov-retailnext thanks for this work. I'll get it into a minor release shortly. |
|
released in 3.1 |
Issue - #357
See test updating deleted elements commit to reproduce the issue in tests
Description:
The radar has a domPool virtual element to save temporarily deleted nodes and reuse them when a new item is added. So if the item is deleted from the items list the item's node can still exist in the virtual dom so the item's component can be still updated by Ember.
This behavior isn't working for computed fields on the ember models. Ember rejects the error when trying to access the computed property on the destroyed object if it hasn't been consumed before. This is possible if the items' component should render more fields after the update.
To avoid re-rendering deleted items it's better to delete related components from the virtual pool. It doesn't give much impact on performance as this can happen only if the component has fewer total items than virtual items after the update and it also doesn't affect scroll rendering.