From 862697d4bd1ab22167e72c14c0f6eefe4cc043d7 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 4 Dec 2018 11:56:24 +0100 Subject: [PATCH] fix(ivy): return new ViewRef when detaching view (#27437) When detaching a view by its index via `ViewContainerRef.detach(index)`, in `ViewEngine` we used to return a new `ViewRef` ([for reference](https://github.com/angular/angular/blob/master/packages/core/src/view/refs.ts#L227)), however in Ivy we return the same `ViewRef` which means that its internal `_viewContainerRef` is never reset and we'll throw an error if the consumer tried to attach it to the `ApplicationRef`. These changes return a new `ViewRef` in order to match the original behavior. These changes also add the same errors as `ViewEngine` when attempting to attach a view that is attached already. This was the original goal of this PR, however it ended up uncovering the issues with the `ViewRef`. PR Close #27437 --- .../core/src/render3/node_manipulation.ts | 4 ++- .../src/render3/view_engine_compatibility.ts | 7 ++-- packages/core/src/render3/view_ref.ts | 14 ++++++-- packages/core/test/application_ref_spec.ts | 34 +++++++++---------- 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 839bada777..2bd2c74b2b 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -369,8 +369,9 @@ export function insertView( * @param lContainer The container from which to detach a view * @param removeIndex The index of the view to detach * @param detached Whether or not this view is already detached. + * @returns Detached LView instance. */ -export function detachView(lContainer: LContainer, removeIndex: number, detached: boolean) { +export function detachView(lContainer: LContainer, removeIndex: number, detached: boolean): LView { const views = lContainer[VIEWS]; const viewToDetach = views[removeIndex]; if (removeIndex > 0) { @@ -388,6 +389,7 @@ export function detachView(lContainer: LContainer, removeIndex: number, detached viewToDetach[PARENT] = null; // Unsets the attached flag viewToDetach[FLAGS] &= ~LViewFlags.Attached; + return viewToDetach; } /** diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index c782f4bfbf..779d1de878 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -25,7 +25,7 @@ import {RenderFlags} from './interfaces/definition'; import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeFlags, TNodeType, TViewNode} from './interfaces/node'; import {LQueries} from './interfaces/query'; import {RComment, RElement, Renderer3, isProceduralRenderer} from './interfaces/renderer'; -import {CONTEXT, HOST_NODE, LView, QUERIES, RENDERER, TView} from './interfaces/view'; +import {CONTAINER_INDEX, CONTEXT, HOST_NODE, LView, QUERIES, RENDERER, TView} from './interfaces/view'; import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert'; import {addRemoveViewFromContainer, appendChild, detachView, findComponentView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode, removeView} from './node_manipulation'; import {getLView, getPreviousOrParentTNode} from './state'; @@ -284,8 +284,9 @@ export function createContainerRef( detach(index?: number): viewEngine_ViewRef|null { const adjustedIdx = this._adjustIndex(index, -1); - detachView(this._lContainer, adjustedIdx, !!this._hostTNode.detached); - return this._viewRefs.splice(adjustedIdx, 1)[0] || null; + const view = detachView(this._lContainer, adjustedIdx, !!this._hostTNode.detached); + const wasDetached = this._viewRefs.splice(adjustedIdx, 1)[0] != null; + return wasDetached ? new ViewRef(view, view[CONTEXT], view[CONTAINER_INDEX]) : null; } private _adjustIndex(index?: number, shift: number = 0) { diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 03ab0186f5..fc6c07161c 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -258,11 +258,21 @@ export class ViewRef implements viewEngine_EmbeddedViewRef, viewEngine_Int */ checkNoChanges(): void { checkNoChanges(this.context); } - attachToViewContainerRef(vcRef: viewEngine_ViewContainerRef) { this._viewContainerRef = vcRef; } + attachToViewContainerRef(vcRef: viewEngine_ViewContainerRef) { + if (this._appRef) { + throw new Error('This view is already attached directly to the ApplicationRef!'); + } + this._viewContainerRef = vcRef; + } detachFromAppRef() { this._appRef = null; } - attachToAppRef(appRef: ApplicationRef) { this._appRef = appRef; } + attachToAppRef(appRef: ApplicationRef) { + if (this._viewContainerRef) { + throw new Error('This view is already attached to a ViewContainer!'); + } + this._appRef = appRef; + } private _lookUpContext(): T { return this._context = this._lView[PARENT] ![this._componentIndex] as T; diff --git a/packages/core/test/application_ref_spec.ts b/packages/core/test/application_ref_spec.ts index cd26571077..508606ce77 100644 --- a/packages/core/test/application_ref_spec.ts +++ b/packages/core/test/application_ref_spec.ts @@ -429,25 +429,25 @@ class SomeComponent { expect(appRef.viewCount).toBe(0); }); - fixmeIvy('unknown') && - it('should not allow to attach a view to both, a view container and the ApplicationRef', - () => { - const comp = TestBed.createComponent(MyComp); - let hostView = comp.componentRef.hostView; - const containerComp = TestBed.createComponent(ContainerComp); - containerComp.detectChanges(); - const vc = containerComp.componentInstance.vc; - const appRef: ApplicationRef = TestBed.get(ApplicationRef); - vc.insert(hostView); - expect(() => appRef.attachView(hostView)) - .toThrowError('This view is already attached to a ViewContainer!'); - hostView = vc.detach(0) !; + it('should not allow to attach a view to both, a view container and the ApplicationRef', + () => { + const comp = TestBed.createComponent(MyComp); + let hostView = comp.componentRef.hostView; + const containerComp = TestBed.createComponent(ContainerComp); + containerComp.detectChanges(); + const vc = containerComp.componentInstance.vc; + const appRef: ApplicationRef = TestBed.get(ApplicationRef); - appRef.attachView(hostView); - expect(() => vc.insert(hostView)) - .toThrowError('This view is already attached directly to the ApplicationRef!'); - }); + vc.insert(hostView); + expect(() => appRef.attachView(hostView)) + .toThrowError('This view is already attached to a ViewContainer!'); + hostView = vc.detach(0) !; + + appRef.attachView(hostView); + expect(() => vc.insert(hostView)) + .toThrowError('This view is already attached directly to the ApplicationRef!'); + }); }); });