From f0f65443c695c07173587aea639d7019dd35adcf Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 5 May 2017 13:50:41 -0700 Subject: [PATCH] fix(core): detach projected views when a parent view is destroyed (#16592) This also clarifies via a test that we only update projected views when the view is created or destroyed, but not when it is attached/detached/moved. Fixes #15578 PR Close #16592 --- packages/core/src/view/types.ts | 7 ++- packages/core/src/view/view_attach.ts | 59 +++++++++++++------ .../test/linker/query_integration_spec.ts | 46 +++++++++++++++ 3 files changed, 90 insertions(+), 22 deletions(-) diff --git a/packages/core/src/view/types.ts b/packages/core/src/view/types.ts index 46811ab273..8746da9c4d 100644 --- a/packages/core/src/view/types.ts +++ b/packages/core/src/view/types.ts @@ -329,9 +329,10 @@ export const enum ViewState { FirstCheck = 1 << 1, Attached = 1 << 2, ChecksEnabled = 1 << 3, - CheckProjectedView = 1 << 4, - CheckProjectedViews = 1 << 5, - Destroyed = 1 << 6, + IsProjectedView = 1 << 4, + CheckProjectedView = 1 << 5, + CheckProjectedViews = 1 << 6, + Destroyed = 1 << 7, CatDetectChanges = Attached | ChecksEnabled, CatInit = BeforeFirstCheck | CatDetectChanges diff --git a/packages/core/src/view/view_attach.ts b/packages/core/src/view/view_attach.ts index a25af16337..31726e3139 100644 --- a/packages/core/src/view/view_attach.ts +++ b/packages/core/src/view/view_attach.ts @@ -18,19 +18,7 @@ export function attachEmbeddedView( } view.viewContainerParent = parentView; addToArray(embeddedViews, viewIndex !, view); - const dvcElementData = declaredViewContainer(view); - if (dvcElementData && dvcElementData !== elementData) { - let projectedViews = dvcElementData.template._projectedViews; - if (!projectedViews) { - projectedViews = dvcElementData.template._projectedViews = []; - } - projectedViews.push(view); - if (view.parent) { - // Note: we are changing the NodeDef here as we cannot calculate - // the fact whether a template is used for projection during compilation. - markNodeAsProjectedTemplate(view.parent.def, view.parentNodeDef !); - } - } + attachProjectedView(elementData, view); Services.dirtyParentQueries(view); @@ -38,6 +26,30 @@ export function attachEmbeddedView( renderAttachEmbeddedView(elementData, prevView, view); } +function attachProjectedView(vcElementData: ElementData, view: ViewData) { + const dvcElementData = declaredViewContainer(view); + if (!dvcElementData || dvcElementData === vcElementData || + view.state & ViewState.IsProjectedView) { + return; + } + // Note: For performance reasons, we + // - add a view to template._projectedViews only 1x throughout its lifetime, + // and remove it not until the view is destroyed. + // (hard, as when a parent view is attached/detached we would need to attach/detach all + // nested projected views as well, even accross component boundaries). + // - don't track the insertion order of views in the projected views array + // (hard, as when the views of the same template are inserted different view containers) + view.state |= ViewState.IsProjectedView; + let projectedViews = dvcElementData.template._projectedViews; + if (!projectedViews) { + projectedViews = dvcElementData.template._projectedViews = []; + } + projectedViews.push(view); + // Note: we are changing the NodeDef here as we cannot calculate + // the fact whether a template is used for projection during compilation. + markNodeAsProjectedTemplate(view.parent !.def, view.parentNodeDef !); +} + function markNodeAsProjectedTemplate(viewDef: ViewDefinition, nodeDef: NodeDef) { if (nodeDef.flags & NodeFlags.ProjectedTemplate) { return; @@ -63,12 +75,7 @@ export function detachEmbeddedView(elementData: ElementData, viewIndex?: number) view.viewContainerParent = null; removeFromArray(embeddedViews, viewIndex); - const dvcElementData = declaredViewContainer(view); - if (dvcElementData && dvcElementData !== elementData) { - const projectedViews = dvcElementData.template._projectedViews; - removeFromArray(projectedViews, projectedViews.indexOf(view)); - } - + // See attachProjectedView for why we don't update projectedViews here. Services.dirtyParentQueries(view); renderDetachView(view); @@ -76,6 +83,20 @@ export function detachEmbeddedView(elementData: ElementData, viewIndex?: number) return view; } +export function detachProjectedView(view: ViewData) { + if (!(view.state & ViewState.IsProjectedView)) { + return; + } + const dvcElementData = declaredViewContainer(view); + if (dvcElementData) { + const projectedViews = dvcElementData.template._projectedViews; + if (projectedViews) { + removeFromArray(projectedViews, projectedViews.indexOf(view)); + Services.dirtyParentQueries(view); + } + } +} + export function moveEmbeddedView( elementData: ElementData, oldViewIndex: number, newViewIndex: number): ViewData { const embeddedViews = elementData.viewContainer !._embeddedViews; diff --git a/packages/core/test/linker/query_integration_spec.ts b/packages/core/test/linker/query_integration_spec.ts index 1b18ab716a..92b8fa0047 100644 --- a/packages/core/test/linker/query_integration_spec.ts +++ b/packages/core/test/linker/query_integration_spec.ts @@ -536,6 +536,52 @@ export function main() { expect(q.query.length).toBe(0); }); + // Note: This tests is just document our current behavior, which we do + // for performance reasons. + it('should not affected queries for projected templates if views are detached or moved', () => { + const template = + '
'; + const view = createTestCmpAndDetectChanges(MyComp0, template); + const q = view.debugElement.children[0].references !['q'] as ManualProjecting; + expect(q.query.length).toBe(0); + + const view1 = q.vc.createEmbeddedView(q.template, {'x': '1'}); + const view2 = q.vc.createEmbeddedView(q.template, {'x': '2'}); + view.detectChanges(); + expect(q.query.map((d: TextDirective) => d.text)).toEqual(['1', '2']); + + q.vc.detach(1); + q.vc.detach(0); + + view.detectChanges(); + expect(q.query.map((d: TextDirective) => d.text)).toEqual(['1', '2']); + + q.vc.insert(view2); + q.vc.insert(view1); + + view.detectChanges(); + expect(q.query.map((d: TextDirective) => d.text)).toEqual(['1', '2']); + }); + + it('should remove manually projected templates if their parent view is destroyed', () => { + const template = ` +
+
+ +
+ `; + const view = createTestCmp(MyComp0, template); + const q = view.debugElement.children[0].references !['q']; + view.componentInstance.shouldShow = true; + view.detectChanges(); + + expect(q.query.length).toBe(1); + + view.componentInstance.shouldShow = false; + view.detectChanges(); + expect(q.query.length).toBe(0); + }); + it('should not throw if a content template is queried and created in the view during change detection', () => { @Component(