From b3ffeaa22bed7679374203fcd4a178b6d0cb13db Mon Sep 17 00:00:00 2001 From: Marc Laval Date: Wed, 21 Feb 2018 15:08:18 +0100 Subject: [PATCH] fix(ivy): OnDestroy hook should not be called twice for a directive on an element (#22350) PR Close #22350 --- .../core/src/render3/node_manipulation.ts | 4 +- packages/core/test/render3/lifecycle_spec.ts | 150 +++++++++++++++++- 2 files changed, 147 insertions(+), 7 deletions(-) diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index d40c92ef620..41a12425eaf 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -233,7 +233,9 @@ export function destroyViewTree(rootView: LView): void { } if (next == null) { - while (viewOrContainer && !viewOrContainer !.next) { + // 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. + while (viewOrContainer && !viewOrContainer !.next && viewOrContainer !== rootView) { cleanUpView(viewOrContainer as LView); viewOrContainer = getParentState(viewOrContainer, rootView); } diff --git a/packages/core/test/render3/lifecycle_spec.ts b/packages/core/test/render3/lifecycle_spec.ts index 799ada0d99f..374b9e6a4f5 100644 --- a/packages/core/test/render3/lifecycle_spec.ts +++ b/packages/core/test/render3/lifecycle_spec.ts @@ -59,6 +59,12 @@ describe('lifecycles', () => { }; } + class Directive { + ngOnInit() { events.push('dir'); } + + static ngDirectiveDef = defineDirective({type: Directive, factory: () => new Directive()}); + } + it('should call onInit method after inputs are set in creation mode (and not in update mode)', () => { /** */ @@ -221,12 +227,7 @@ describe('lifecycles', () => { }); it('should be called on directives after component', () => { - class Directive { - ngOnInit() { events.push('dir'); } - - static ngDirectiveDef = defineDirective({type: Directive, factory: () => new Directive()}); - } - + /** */ function Template(ctx: any, cm: boolean) { if (cm) { elementStart(0, Comp, null, [Directive]); @@ -246,6 +247,24 @@ describe('lifecycles', () => { }); + it('should be called on directives on an element', () => { + /**
*/ + function Template(ctx: any, cm: boolean) { + if (cm) { + elementStart(0, 'div', null, [Directive]); + elementEnd(); + } + Directive.ngDirectiveDef.h(1, 0); + componentRefresh(1, 0); + } + + renderToHtml(Template, {}); + expect(events).toEqual(['dir']); + + renderToHtml(Template, {}); + expect(events).toEqual(['dir']); + }); + it('should call onInit properly in for loop', () => { /** * @@ -370,6 +389,12 @@ describe('lifecycles', () => { }; } + class Directive { + ngDoCheck() { events.push('dir'); } + + static ngDirectiveDef = defineDirective({type: Directive, factory: () => new Directive()}); + } + it('should call doCheck on every refresh', () => { /** */ function Template(ctx: any, cm: boolean) { @@ -425,6 +450,45 @@ describe('lifecycles', () => { expect(allEvents).toEqual(['init comp', 'check comp', 'check comp']); }); + it('should be called on directives after component', () => { + /** */ + function Template(ctx: any, cm: boolean) { + if (cm) { + elementStart(0, Comp, null, [Directive]); + elementEnd(); + } + Comp.ngComponentDef.h(1, 0); + Directive.ngDirectiveDef.h(2, 0); + componentRefresh(1, 0); + componentRefresh(2, 0); + } + + renderToHtml(Template, {}); + expect(events).toEqual(['comp', 'dir']); + + renderToHtml(Template, {}); + expect(events).toEqual(['comp', 'dir', 'comp', 'dir']); + + }); + + it('should be called on directives on an element', () => { + /**
*/ + function Template(ctx: any, cm: boolean) { + if (cm) { + elementStart(0, 'div', null, [Directive]); + elementEnd(); + } + Directive.ngDirectiveDef.h(1, 0); + componentRefresh(1, 0); + } + + renderToHtml(Template, {}); + expect(events).toEqual(['dir']); + + renderToHtml(Template, {}); + expect(events).toEqual(['dir', 'dir']); + }); + }); describe('afterContentInit', () => { @@ -1324,6 +1388,12 @@ describe('lifecycles', () => { }; } + class Directive { + ngOnDestroy() { events.push('dir'); } + + static ngDirectiveDef = defineDirective({type: Directive, factory: () => new Directive()}); + } + it('should call destroy when view is removed', () => { /** * % if (condition) { @@ -1747,6 +1817,74 @@ describe('lifecycles', () => { expect(ctx.counter).toEqual(2); }); + it('should be called on directives after component', () => { + /** + * % if (condition) { + * + * % } + */ + + function Template(ctx: any, cm: boolean) { + if (cm) { + container(0); + } + containerRefreshStart(0); + { + if (ctx.condition) { + if (embeddedViewStart(0)) { + elementStart(0, Comp, null, [Directive]); + elementEnd(); + } + Comp.ngComponentDef.h(1, 0); + Directive.ngDirectiveDef.h(2, 0); + componentRefresh(1, 0); + componentRefresh(2, 0); + embeddedViewEnd(); + } + } + containerRefreshEnd(); + } + + renderToHtml(Template, {condition: true}); + expect(events).toEqual([]); + + renderToHtml(Template, {condition: false}); + expect(events).toEqual(['comp', 'dir']); + + }); + + it('should be called on directives on an element', () => { + /** + * % if (condition) { + *
+ * % } + */ + + function Template(ctx: any, cm: boolean) { + if (cm) { + container(0); + } + containerRefreshStart(0); + { + if (ctx.condition) { + if (embeddedViewStart(0)) { + elementStart(0, 'div', null, [Directive]); + elementEnd(); + } + Directive.ngDirectiveDef.h(1, 0); + componentRefresh(1, 0); + embeddedViewEnd(); + } + } + containerRefreshEnd(); + } + + renderToHtml(Template, {condition: true}); + expect(events).toEqual([]); + + renderToHtml(Template, {condition: false}); + expect(events).toEqual(['dir']); + }); });