From 5632424d04b5bb25a0243153e0ce4a7dd673723d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=A1ko=20Hevery?= Date: Wed, 9 Oct 2019 16:58:01 -0700 Subject: [PATCH] refactor(ivy): ViewRef needs embededViewRef declaration (#33074) PR Close #33074 --- .../src/render3/view_engine_compatibility.ts | 33 ++++++++------- packages/core/src/render3/view_ref.ts | 41 +++++++++++-------- .../test/acceptance/change_detection_spec.ts | 36 ++++++++++++++++ .../cyclic_import/bundle.golden_symbols.json | 2 +- .../hello_world/bundle.golden_symbols.json | 2 +- .../bundling/todo/bundle.golden_symbols.json | 2 +- 6 files changed, 83 insertions(+), 33 deletions(-) diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index 5e062c7626..65aefe14e6 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -122,7 +122,7 @@ export function createTemplateRef( renderView(lView, embeddedTView, context); - const viewRef = new ViewRef(lView, context, -1); + const viewRef = new ViewRef(lView); viewRef._tViewNode = lView[T_HOST] as TViewNode; return viewRef; } @@ -295,7 +295,7 @@ export function createContainerRef( const wasDetached = view && removeFromArray(this._lContainer[VIEW_REFS] !, adjustedIdx) != null; - return wasDetached ? new ViewRef(view !, view ![CONTEXT], -1) : null; + return wasDetached ? new ViewRef(view !) : null; } private _adjustIndex(index?: number, shift: number = 0) { @@ -370,22 +370,27 @@ export function injectChangeDetectorRef(isPipe = false): ViewEngine_ChangeDetect /** * Creates a ViewRef and stores it on the injector as ChangeDetectorRef (public alias). * - * @param hostTNode The node that is requesting a ChangeDetectorRef - * @param hostView The view to which the node belongs + * @param tNode The node that is requesting a ChangeDetectorRef + * @param lView The view to which the node belongs * @param isPipe Whether the view is being injected into a pipe. * @returns The ChangeDetectorRef to use */ -function createViewRef( - hostTNode: TNode, hostView: LView, isPipe: boolean): ViewEngine_ChangeDetectorRef { - if (isComponentHost(hostTNode) && !isPipe) { - const componentIndex = hostTNode.directiveStart; - const componentView = getComponentLViewByIndex(hostTNode.index, hostView); - return new ViewRef(componentView, null, componentIndex); +function createViewRef(tNode: TNode, lView: LView, isPipe: boolean): ViewEngine_ChangeDetectorRef { + // `isComponentView` will be true for Component and Directives (but not for Pipes). + // See https://github.com/angular/angular/pull/33072 for proper fix + const isComponentView = !isPipe && isComponentHost(tNode); + if (isComponentView) { + // The LView represents the location where the component is declared. + // Instead we want the LView for the component View and so we need to look it up. + const componentView = getComponentLViewByIndex(tNode.index, lView); // look down + return new ViewRef(componentView, componentView); } else if ( - hostTNode.type === TNodeType.Element || hostTNode.type === TNodeType.Container || - hostTNode.type === TNodeType.ElementContainer) { - const hostComponentView = findComponentView(hostView); - return new ViewRef(hostComponentView, hostComponentView[CONTEXT], -1); + tNode.type === TNodeType.Element || tNode.type === TNodeType.Container || + tNode.type === TNodeType.ElementContainer) { + // The LView represents the location where the injection is requested from. + // We need to locate the containing LView (in case where the `lView` is an embedded view) + const hostComponentView = findComponentView(lView); // look up + return new ViewRef(hostComponentView, lView); } return null !; } diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 0b177726d6..1dbd321106 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -13,7 +13,7 @@ import {EmbeddedViewRef as viewEngine_EmbeddedViewRef, InternalViewRef as viewEn import {checkNoChangesInRootView, checkNoChangesInternal, detectChangesInRootView, detectChangesInternal, markViewDirty, storeCleanupFn} from './instructions/shared'; import {TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node'; -import {FLAGS, HOST, LView, LViewFlags, T_HOST} from './interfaces/view'; +import {CONTEXT, FLAGS, HOST, LView, LViewFlags, T_HOST} from './interfaces/view'; import {destroyLView, renderDetachView} from './node_manipulation'; import {findComponentView, getLViewParent} from './util/view_traversal_utils'; import {getNativeByTNode, getNativeByTNodeOrNull} from './util/view_utils'; @@ -35,11 +35,6 @@ export class ViewRef implements viewEngine_EmbeddedViewRef, viewEngine_Int */ public _tViewNode: TViewNode|null = null; - /** - * @internal - */ - public _lView: LView; - get rootNodes(): any[] { if (this._lView[HOST] == null) { const tView = this._lView[T_HOST] as TViewNode; @@ -48,11 +43,29 @@ export class ViewRef implements viewEngine_EmbeddedViewRef, viewEngine_Int return []; } - constructor(_lView: LView, private _context: T|null, private _componentIndex: number) { - this._lView = _lView; - } + constructor( + /** + * This represents `LView` associated with the component when ViewRef is a ChangeDetectorRef. + * + * When ViewRef is created for a dynamic component, this also represents the `LView` for the + * component. + * + * For a "regular" ViewRef created for an embedded view, this is the `LView` for the embedded + * view. + * + * @internal + */ + public _lView: LView, - get context(): T { return this._context ? this._context : this._lookUpContext(); } + /** + * This represents the `LView` associated with the point where `ChangeDetectorRef` was + * requested. + * + * This may be different from `_lView` if the `_cdRefInjectingView` is an embedded view. + */ + private _cdRefInjectingView?: LView) {} + + get context(): T { return this._lView[CONTEXT] as T; } get destroyed(): boolean { return (this._lView[FLAGS] & LViewFlags.Destroyed) === LViewFlags.Destroyed; @@ -109,7 +122,7 @@ export class ViewRef implements viewEngine_EmbeddedViewRef, viewEngine_Int * } * ``` */ - markForCheck(): void { markViewDirty(this._lView); } + markForCheck(): void { markViewDirty(this._cdRefInjectingView || this._lView); } /** * Detaches the view from the change detection tree. @@ -273,15 +286,11 @@ export class ViewRef implements viewEngine_EmbeddedViewRef, viewEngine_Int } this._appRef = appRef; } - - private _lookUpContext(): T { - return this._context = getLViewParent(this._lView) ![this._componentIndex] as T; - } } /** @internal */ export class RootViewRef extends ViewRef { - constructor(public _view: LView) { super(_view, null, -1); } + constructor(public _view: LView) { super(_view); } detectChanges(): void { detectChangesInRootView(this._view); } diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index 8cca530f35..e947a1befe 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -11,6 +11,7 @@ import {CommonModule} from '@angular/common'; import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, Input, NgModule, OnInit, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; +import {BehaviorSubject} from 'rxjs'; describe('change detection', () => { @@ -913,6 +914,41 @@ describe('change detection', () => { expect(fixture.nativeElement.textContent).toEqual('two - two'); }); + it('async pipe should trigger CD for embedded views where the declaration and insertion views are different', + () => { + @Component({ + selector: 'insertion', + changeDetection: ChangeDetectionStrategy.OnPush, + template: ` ` + }) + class Insertion { + @Input() template !: TemplateRef<{}>; + } + + // This component uses async pipe (which calls markForCheck) in a view that has different + // insertion and declaration views. + @Component({ + changeDetection: ChangeDetectionStrategy.OnPush, + template: ` + + + {{value | async}} + + ` + }) + class Declaration { + value = new BehaviorSubject('initial value'); + } + + const fixture = TestBed.configureTestingModule({declarations: [Insertion, Declaration]}) + .createComponent(Declaration); + fixture.detectChanges(); + expect(fixture.debugElement.nativeElement.textContent).toContain('initial value'); + fixture.componentInstance.value.next('new value'); + fixture.detectChanges(); + expect(fixture.debugElement.nativeElement.textContent).toContain('new value'); + }); + // TODO(kara): add test for dynamic views once bug fix is in }); diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index b2ff9ceaa6..58ce0ef3ba 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -282,7 +282,7 @@ "name": "getComponentDef" }, { - "name": "getComponentViewByIndex" + "name": "getComponentLViewByIndex" }, { "name": "getContainerRenderParent" diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 5da2c4dc39..7758a17a73 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -225,7 +225,7 @@ "name": "getComponentDef" }, { - "name": "getComponentViewByIndex" + "name": "getComponentLViewByIndex" }, { "name": "getContainerRenderParent" diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index b0c792bc1b..02ba8c6a28 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -621,7 +621,7 @@ "name": "getComponentDef" }, { - "name": "getComponentViewByIndex" + "name": "getComponentLViewByIndex" }, { "name": "getComponentViewByInstance"