fix(ivy): properly destroy view trees where root is an embedded view without children (#23482)

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.

PR Close #23482
This commit is contained in:
Pawel Kozlowski 2018-04-19 22:59:58 +02:00 committed by Victor Berchet
parent 06c0d9666f
commit b1d03fe70b
2 changed files with 88 additions and 5 deletions

View File

@ -209,7 +209,7 @@ export function addRemoveViewFromContainer(
} }
/** /**
* Traverses the tree of component views and containers to remove listeners and * Traverses down and up the tree of views and containers to remove listeners and
* call onDestroy callbacks. * call onDestroy callbacks.
* *
* Notes: * Notes:
@ -222,7 +222,11 @@ export function addRemoveViewFromContainer(
* @param rootView The view to destroy * @param rootView The view to destroy
*/ */
export function destroyViewTree(rootView: LView): void { export function destroyViewTree(rootView: LView): void {
let viewOrContainer: LViewOrLContainer|null = rootView; // A view to cleanup doesn't have children so we should not try to descend down the view tree.
if (!rootView.child) {
return cleanUpView(rootView);
}
let viewOrContainer: LViewOrLContainer|null = rootView.child;
while (viewOrContainer) { while (viewOrContainer) {
let next: LViewOrLContainer|null = null; let next: LViewOrLContainer|null = null;
@ -232,13 +236,16 @@ export function destroyViewTree(rootView: LView): void {
} else if (viewOrContainer.child) { } else if (viewOrContainer.child) {
next = viewOrContainer.child; next = viewOrContainer.child;
} else if (viewOrContainer.next) { } else if (viewOrContainer.next) {
// Only move to the side and clean if operating below rootView -
// otherwise we would start cleaning up sibling views of the rootView.
cleanUpView(viewOrContainer as LView); cleanUpView(viewOrContainer as LView);
next = viewOrContainer.next; next = viewOrContainer.next;
} }
if (next == null) { if (next == null) {
// If the viewOrContainer is the rootView, then the cleanup is done twice. // If the viewOrContainer is the rootView and next is null it means that we are dealing
// Without this check, ngOnDestroy would be called twice for a directive on an element. // with a root view that doesn't have children. We didn't descend into child views
// so no need to go back up the views tree.
while (viewOrContainer && !viewOrContainer !.next && viewOrContainer !== rootView) { while (viewOrContainer && !viewOrContainer !.next && viewOrContainer !== rootView) {
cleanUpView(viewOrContainer as LView); cleanUpView(viewOrContainer as LView);
viewOrContainer = getParentState(viewOrContainer, rootView); viewOrContainer = getParentState(viewOrContainer, rootView);

View File

@ -9,8 +9,9 @@
import {NgForOfContext} from '@angular/common'; import {NgForOfContext} from '@angular/common';
import {defineComponent} from '../../src/render3/index'; import {defineComponent} from '../../src/render3/index';
import {bind, container, elementEnd, elementProperty, elementStart, interpolation3, text, textBinding, tick} from '../../src/render3/instructions'; import {bind, container, elementEnd, elementProperty, elementStart, interpolation1, interpolation3, listener, text, textBinding, tick} from '../../src/render3/instructions';
import {RenderFlags} from '../../src/render3/interfaces/definition'; import {RenderFlags} from '../../src/render3/interfaces/definition';
import {NgForOf, NgIf} from './common_with_def'; import {NgForOf, NgIf} from './common_with_def';
import {ComponentFixture} from './render_util'; import {ComponentFixture} from './render_util';
@ -123,6 +124,81 @@ describe('@angular/common integration', () => {
.toEqual('<ul><li>0 of 3: first</li><li>1 of 3: middle</li><li>2 of 3: second</li></ul>'); .toEqual('<ul><li>0 of 3: first</li><li>1 of 3: middle</li><li>2 of 3: second</li></ul>');
}); });
it('should retain parent view listeners when the NgFor destroy views', () => {
class MyApp {
private _data: number[] = [1, 2, 3];
items: number[] = [];
toggle() {
if (this.items.length) {
this.items = [];
} else {
this.items = this._data;
}
}
static ngComponentDef = defineComponent({
type: MyApp,
factory: () => new MyApp(),
selectors: [['my-app']],
// <button (click)="toggle()">Toggle List</button>
// <ul>
// <li *ngFor="let item of items">{{index}}</li>
// </ul>
template: (rf: RenderFlags, myApp: MyApp) => {
if (rf & RenderFlags.Create) {
elementStart(0, 'button');
{
listener('click', function() { return myApp.toggle(); });
text(1, 'Toggle List');
}
elementEnd();
elementStart(2, 'ul');
{ container(3, liTemplate, undefined, ['ngForOf', '']); }
elementEnd();
}
if (rf & RenderFlags.Update) {
elementProperty(3, 'ngForOf', bind(myApp.items));
}
function liTemplate(rf1: RenderFlags, row: NgForOfContext<string>) {
if (rf1 & RenderFlags.Create) {
elementStart(0, 'li');
{ text(1); }
elementEnd();
}
if (rf1 & RenderFlags.Update) {
textBinding(1, interpolation1('', row.$implicit, ''));
}
}
},
directives: () => [NgForOf]
});
}
const fixture = new ComponentFixture(MyApp);
const button = fixture.hostElement.querySelector('button') !;
expect(fixture.html).toEqual('<button>Toggle List</button><ul></ul>');
// this will fill the list
fixture.component.toggle();
fixture.update();
expect(fixture.html)
.toEqual('<button>Toggle List</button><ul><li>1</li><li>2</li><li>3</li></ul>');
button.click();
fixture.update();
expect(fixture.html).toEqual('<button>Toggle List</button><ul></ul>');
button.click();
fixture.update();
expect(fixture.html)
.toEqual('<button>Toggle List</button><ul><li>1</li><li>2</li><li>3</li></ul>');
});
}); });
describe('ngIf', () => { describe('ngIf', () => {