fix(ivy): properly destroy view trees where root is an embedded view … #23482
Conversation
pkozlowski-opensource
added
target: major
pkozlowski-opensource
force-pushed
the
vcref_view_tree
branch
from
ea2e5e2
to
14dea45
Compare
|
You can preview ea2e5e2 at https://pr23482-ea2e5e2.ngbuilds.io/. |
|
You can preview 14dea45 at https://pr23482-14dea45.ngbuilds.io/. |
| selectors: [['my-app']], | ||
| // <button (click)="toggle()">Toggle List</button> | ||
| // <ul> | ||
| // <li *ngFor="let item of items">{{index}} of {{count}}: {{item}}</li> |
There was a problem hiding this comment.
The comment here is off from the actual liTemplate template function, in which only {{item}} is used as li's text content. Also, why not just include this in an @Component annotation?
| @@ -231,21 +231,24 @@ export function destroyViewTree(rootView: LView): void { | |||
| next = viewOrContainer.views[0].data; | |||
| } else if (viewOrContainer.child) { | |||
| next = viewOrContainer.child; | |||
| } else if (viewOrContainer.next) { | |||
| } else if (viewOrContainer.next && viewOrContainer !== rootView) { | |||
There was a problem hiding this comment.
I think we should be able to avoid adding this check and the one on line 251 if we return early for the case that the root view has no children. e.g.
function destroyViewTree(rootView: LView): void {
if (!rootView.child) return cleanUpView(rootView);
let viewOrContainer: LViewOrLContainer|null = rootView.child;
...
}There was a problem hiding this comment.
Oh, right, great 👍 - those repeated checks were not nice, this is much better! Amending the PR.
pkozlowski-opensource
force-pushed
the
vcref_view_tree
branch
2 times, most recently
from
d034923
to
4132eae
Compare
pkozlowski-opensource
added
the
action: review
|
Thnx for review @kara - amended the PR based on your suggestion. Please have another look / approve. |
| // If the viewOrContainer is the rootView, then the cleanup is done twice. | ||
| // Without this check, ngOnDestroy would be called twice for a directive on an element. | ||
| // If the viewOrContainer is the rootView and next is null it means that we are dealing | ||
| // with a root view that doesn't have children. We didn't descent into child views |
…without children The bug fixed here steams from the fact that we are traversing too far up in the views tree hierarchy in the destroyViewTree function. The logic in destroyViewTree is off if we start removal at an embedded view without any child views. For such a case we should just clean up (cleanUpView) this one view without paying attention to next / parent views.
pkozlowski-opensource
force-pushed
the
vcref_view_tree
branch
from
4132eae
to
718c93d
Compare
pkozlowski-opensource
removed
the
action: review
|
You can preview 718c93d at https://pr23482-718c93d.ngbuilds.io/. |
pkozlowski-opensource
added
the
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…without children
The bug fixed here steams from the fact that we are traversing too far up
in the views tree hierarchy in the destroyViewTree function.
The logic in destroyViewTree is off if we start removal at an embedded view
without any child views. For such a case we should just clean up (cleanUpView)
this one view without paying attention to next / parent views.
@kara I very much dislike how the solution in this PR keeps repeating
!== rootViewbut didn't come up with anything better without fully understanding thedestroyViewTree. Let's discuss more in the office