From b62b11bd6b239e62e475fcf3fe12c4d9a0738f7e Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Thu, 7 Nov 2019 06:32:59 +0000 Subject: [PATCH] fix(ivy): Run ChangeDetection on transplanted views (#33644) https://hackmd.io/@mhevery/rJUJsvv9H Closes #33393 PR Close #33644 --- integration/_payload-limits.json | 2 +- .../core/src/render3/instructions/shared.ts | 56 +++++-- .../core/src/render3/node_manipulation.ts | 13 +- .../src/render3/util/view_traversal_utils.ts | 7 +- .../src/render3/view_engine_compatibility.ts | 4 +- .../test/acceptance/change_detection_spec.ts | 142 +++++++++++++++++- .../cyclic_import/bundle.golden_symbols.json | 3 + .../hello_world/bundle.golden_symbols.json | 3 + packages/core/test/render3/perf/setup.ts | 2 +- packages/core/test/render3/view_utils_spec.ts | 2 +- 10 files changed, 204 insertions(+), 30 deletions(-) diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index e6187cce9b..1bc93012dc 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 15040, + "main-es2015": 15267, "polyfills-es2015": 36808 } } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index a172c80a3a..79dbfb9be5 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -10,7 +10,7 @@ import {ErrorHandler} from '../../error_handler'; import {CUSTOM_ELEMENTS_SCHEMA, NO_ERRORS_SCHEMA, SchemaMetadata} from '../../metadata/schema'; import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../../sanitization/sanitization'; import {Sanitizer} from '../../sanitization/sanitizer'; -import {assertDataInRange, assertDefined, assertDomNode, assertEqual, assertGreaterThan, assertLessThanOrEqual, assertNotEqual, assertNotSame} from '../../util/assert'; +import {assertDataInRange, assertDefined, assertDomNode, assertEqual, assertGreaterThan, assertNotEqual, assertNotSame} from '../../util/assert'; import {createNamedArrayType} from '../../util/named_array_type'; import {initNgDevMode} from '../../util/ng_dev_mode'; import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect'; @@ -20,7 +20,7 @@ import {getFactoryDef} from '../definition'; import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from '../di'; import {throwMultipleComponentError} from '../errors'; import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags, registerPreOrderHooks} from '../hooks'; -import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer} from '../interfaces/container'; +import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer, MOVED_VIEWS} from '../interfaces/container'; import {ComponentDef, ComponentTemplate, DirectiveDef, DirectiveDefListOrFactory, PipeDefListOrFactory, RenderFlags, ViewQueriesFunction} from '../interfaces/definition'; import {INJECTOR_BLOOM_PARENT_SIZE, NodeInjectorFactory} from '../interfaces/injector'; import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, PropertyAliasValue, PropertyAliases, TAttributes, TConstants, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TIcuContainerNode, TNode, TNodeFlags, TNodeProviderIndexes, TNodeType, TProjectionNode, TViewNode} from '../interfaces/node'; @@ -30,13 +30,13 @@ import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRoo import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_VIEW, ExpandoInstructions, FLAGS, HEADER_OFFSET, HOST, INJECTOR, InitPhaseState, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, TData, TVIEW, TView, TViewType, T_HOST} from '../interfaces/view'; import {assertNodeOfPossibleTypes} from '../node_assert'; import {isNodeMatchingSelectorList} from '../node_selector_matcher'; -import {ActiveElementFlags, enterView, executeElementExitFn, getBindingIndex, getBindingsEnabled, getCheckNoChangesMode, getIsParent, getPreviousOrParentTNode, getSelectedIndex, hasActiveElementFlag, incrementActiveDirectiveId, leaveView, leaveViewProcessExit, setActiveHostElement, setBindingIndex, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; +import {ActiveElementFlags, enterView, executeElementExitFn, getBindingsEnabled, getCheckNoChangesMode, getIsParent, getPreviousOrParentTNode, getSelectedIndex, hasActiveElementFlag, incrementActiveDirectiveId, leaveView, leaveViewProcessExit, setActiveHostElement, setBindingIndex, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; import {renderStylingMap, writeStylingValueDirectly} from '../styling/bindings'; import {NO_CHANGE} from '../tokens'; import {isAnimationProp} from '../util/attrs_utils'; import {INTERPOLATION_DELIMITER, renderStringify, stringifyForError} from '../util/misc_utils'; import {getInitialStylingValue} from '../util/styling_utils'; -import {getLViewParent} from '../util/view_traversal_utils'; +import {findComponentView, getLViewParent} from '../util/view_traversal_utils'; import {getComponentLViewByIndex, getNativeByIndex, getNativeByTNode, getTNode, isCreationMode, readPatchedLView, resetPreOrderHookFlags, unwrapRNode, viewAttachedToChangeDetector} from '../util/view_utils'; import {selectIndexInternal} from './advance'; @@ -1444,21 +1444,21 @@ const LContainerArray: any = ((typeof ngDevMode === 'undefined' || ngDevMode) && * @returns LContainer */ export function createLContainer( - hostNative: RElement | RComment | LView, currentView: LView, native: RComment, tNode: TNode, - isForViewContainerRef?: boolean): LContainer { + hostNative: RElement | RComment | LView, currentView: LView, native: RComment, + tNode: TNode): LContainer { ngDevMode && assertLView(currentView); ngDevMode && !isProceduralRenderer(currentView[RENDERER]) && assertDomNode(native); // https://jsperf.com/array-literal-vs-new-array-really const lContainer: LContainer = new (ngDevMode ? LContainerArray : Array)( - hostNative, // host native - true, // Boolean `true` in this position signifies that this is an `LContainer` - isForViewContainerRef ? -1 : 0, // active index - currentView, // parent - null, // next - null, // queries - tNode, // t_host - native, // native, - null, // view refs + hostNative, // host native + true, // Boolean `true` in this position signifies that this is an `LContainer` + -1, // active index + currentView, // parent + null, // next + null, // queries + tNode, // t_host + native, // native, + null, // view refs ); ngDevMode && attachLContainerDebug(lContainer); return lContainer; @@ -1481,6 +1481,32 @@ function refreshDynamicEmbeddedViews(lView: LView) { ngDevMode && assertDefined(embeddedTView, 'TView must be allocated'); refreshView(embeddedLView, embeddedTView, embeddedTView.template, embeddedLView[CONTEXT] !); } + const movedViews = viewOrContainer[MOVED_VIEWS]; + if (movedViews !== null) { + // We should only CD moved views if the component where they were inserted does not match + // the component where they were declared. Moved views also contains intra component moves, + // which we don't care about. + // TODO(misko): this is not the most efficient way to do this as we have to do a lot of + // searches. Will refactor for performance later. + const declaredComponentLView = findComponentView(lView); + for (let i = 0; i < movedViews.length; i++) { + const movedLView = movedViews[i] !; + let parentLView = movedLView[PARENT]; + while (isLContainer(parentLView)) { + parentLView = parentLView[PARENT]; + } + const insertedComponentLView = findComponentView(parentLView !); + const insertionIsOnPush = + (insertedComponentLView[FLAGS] & LViewFlags.CheckAlways) !== LViewFlags.CheckAlways; + if (insertionIsOnPush && insertedComponentLView !== declaredComponentLView) { + // Here we know that the template has been transplanted across components + // (not just moved within a component) + const movedTView = movedLView[TVIEW]; + ngDevMode && assertDefined(movedTView, 'TView must be allocated'); + refreshView(movedLView, movedTView, movedTView.template, movedLView[CONTEXT] !); + } + } + } } viewOrContainer = viewOrContainer[NEXT]; } diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index c50b3f5bdf..1b41b9f9b1 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -256,12 +256,13 @@ export function insertView(lView: LView, lContainer: LContainer, index: number) * different LContainer. */ function trackMovedView(declarationContainer: LContainer, lView: LView) { + ngDevMode && assertDefined(lView, 'LView required'); ngDevMode && assertLContainer(declarationContainer); - const declaredViews = declarationContainer[MOVED_VIEWS]; - if (declaredViews === null) { + const movedViews = declarationContainer[MOVED_VIEWS]; + if (movedViews === null) { declarationContainer[MOVED_VIEWS] = [lView]; } else { - declaredViews.push(lView); + movedViews.push(lView); } } @@ -270,9 +271,9 @@ function detachMovedView(declarationContainer: LContainer, lView: LView) { ngDevMode && assertDefined( declarationContainer[MOVED_VIEWS], 'A projected view should belong to a non-empty projected views collection'); - const projectedViews = declarationContainer[MOVED_VIEWS] !; - const declaredViewIndex = projectedViews.indexOf(lView); - projectedViews.splice(declaredViewIndex, 1); + const movedViews = declarationContainer[MOVED_VIEWS] !; + const declaredViewIndex = movedViews.indexOf(lView); + movedViews.splice(declaredViewIndex, 1); } /** diff --git a/packages/core/src/render3/util/view_traversal_utils.ts b/packages/core/src/render3/util/view_traversal_utils.ts index 68947854cc..18493189d1 100644 --- a/packages/core/src/render3/util/view_traversal_utils.ts +++ b/packages/core/src/render3/util/view_traversal_utils.ts @@ -56,9 +56,10 @@ export function getRootView(componentOrLView: LView | {}): LView { */ export function findComponentView(lView: LView): LView { let rootTNode = lView[T_HOST]; - while (rootTNode !== null && rootTNode.type === TNodeType.View) { - ngDevMode && assertDefined(lView[DECLARATION_VIEW], 'lView[DECLARATION_VIEW]'); - lView = lView[DECLARATION_VIEW] !; + let declaredView: LView|null; + while (rootTNode !== null && rootTNode.type === TNodeType.View && + (declaredView = lView[DECLARATION_VIEW]) !== null) { + lView = declaredView; rootTNode = lView[T_HOST]; } ngDevMode && assertLView(lView); diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index 65aefe14e6..f877ac13d0 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -25,7 +25,7 @@ import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer, VIEW_REFS} from './in import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node'; import {RComment, RElement, isProceduralRenderer} from './interfaces/renderer'; import {isComponentHost, isLContainer, isLView, isRootView} from './interfaces/type_checks'; -import {CONTEXT, DECLARATION_LCONTAINER, LView, LViewFlags, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view'; +import {DECLARATION_LCONTAINER, LView, LViewFlags, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view'; import {assertNodeOfPossibleTypes} from './node_assert'; import {addRemoveViewFromContainer, appendChild, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode, removeView} from './node_manipulation'; import {getParentInjectorTNode} from './node_util'; @@ -353,7 +353,7 @@ export function createContainerRef( } hostView[hostTNode.index] = lContainer = - createLContainer(slotValue, hostView, commentNode, hostTNode, true); + createLContainer(slotValue, hostView, commentNode, hostTNode); addToViewTree(hostView, lContainer); } diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index ad51503669..a9548bd91f 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -9,7 +9,11 @@ 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 {ivyEnabled} from '@angular/core/src/ivy_switch'; +import {markDirty} from '@angular/core/src/render3/index'; +import {CONTEXT} from '@angular/core/src/render3/interfaces/view'; +import {getCheckNoChangesMode, instructionState} from '@angular/core/src/render3/state'; +import {ComponentFixture, TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {BehaviorSubject} from 'rxjs'; @@ -1098,4 +1102,140 @@ describe('change detection', () => { }); + describe('transplanted views', () => { + @Component({ + selector: 'insert-comp', + changeDetection: ChangeDetectionStrategy.OnPush, + template: ` + InsertComp({{greeting}}) + + + ` + }) + class InsertComp { + get template(): TemplateRef { return declareComp.myTmpl; } + greeting: string = 'Hello'; + constructor(public changeDetectorRef: ChangeDetectorRef) { insertComp = this; } + } + + @Component({ + selector: `declare-comp`, + template: ` + DeclareComp({{name}}) + + {{greeting}} {{nameWithLog}}! + + ` + }) + class DeclareComp { + @ViewChild('myTmpl') + myTmpl !: TemplateRef; + name: string = 'world'; + get nameWithLog() { + if (ivyEnabled && !getCheckNoChangesMode()) { + const lFrame = instructionState.lFrame; + const parentChangeDetectionLView = lFrame.parent.lView; + if (parentChangeDetectionLView[CONTEXT] === declareComp) { + log.push('Declare'); + } else if (parentChangeDetectionLView[CONTEXT] === insertComp) { + log.push('Insert'); + } else { + log.push( + 'UNEXPECTED VIEW: ' + parentChangeDetectionLView ![CONTEXT] !.constructor.name); + } + } + return this.name; + } + constructor() { declareComp = this; } + } + + @Component({ + template: ` + + + ` + }) + class AppComp { + showDeclare: boolean = true; + showInsert: boolean = true; + constructor() { appComp = this; } + } + + let log !: string[]; + let fixture !: ComponentFixture; + let appComp !: AppComp; + let insertComp !: InsertComp; + let declareComp !: DeclareComp; + + beforeEach(() => { + TestBed.configureTestingModule({ + declarations: [InsertComp, DeclareComp, AppComp], + imports: [CommonModule], + }); + log = []; + fixture = TestBed.createComponent(AppComp); + }); + + it('should CD with declaration', () => { + fixture.detectChanges(); + ivyEnabled && expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(world) InsertComp(Hello) Hello world!'); + + declareComp.name = 'Angular'; + fixture.detectChanges(); + ivyEnabled && expect(log).toEqual(['Declare']); + log.length = 0; + // Expect transplanted LView to be CD because the declaration is CD. + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); + + insertComp.greeting = 'Hi'; + fixture.detectChanges(); + ivyEnabled && expect(log).toEqual(['Declare']); + log.length = 0; + // expect no change because it is on push. + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); + + insertComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(); + ivyEnabled && expect(log).toEqual(['Declare', 'Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(Angular) InsertComp(Hi) Hi Angular!'); + + // Destroy insertion should also destroy declaration + appComp.showInsert = false; + insertComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(); + ivyEnabled && expect(log).toEqual([]); // No update in declaration + log.length = 0; + expect(trim(fixture.nativeElement.textContent)).toEqual('DeclareComp(Angular)'); + + // Restore both + appComp.showInsert = true; + fixture.detectChanges(); + ivyEnabled && expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); + + // Destroy declaration, But we should still be able to see updates in insertion + appComp.showDeclare = false; + insertComp.greeting = 'Hello'; + insertComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(); + ivyEnabled && expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)).toEqual('InsertComp(Hello) Hello Angular!'); + }); + }); }); + +function trim(text: string | null): string { + return text ? text.replace(/[\s\n]+/gm, ' ').trim() : ''; +} 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 01ae439028..e7a7a8772c 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -56,6 +56,9 @@ { "name": "MONKEY_PATCH_KEY_NAME" }, + { + "name": "MOVED_VIEWS" + }, { "name": "Module" }, 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 e233d9d0f1..0980b28f7d 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -53,6 +53,9 @@ { "name": "MONKEY_PATCH_KEY_NAME" }, + { + "name": "MOVED_VIEWS" + }, { "name": "NATIVE" }, diff --git a/packages/core/test/render3/perf/setup.ts b/packages/core/test/render3/perf/setup.ts index 67ea569d7b..dcf115e373 100644 --- a/packages/core/test/render3/perf/setup.ts +++ b/packages/core/test/render3/perf/setup.ts @@ -56,7 +56,7 @@ export function setupTestHarness( rendererFactory, renderer); const mockRCommentNode = renderer.createComment(''); const lContainer = - createLContainer(mockRCommentNode, hostLView, mockRCommentNode, tContainerNode, true); + createLContainer(mockRCommentNode, hostLView, mockRCommentNode, tContainerNode); addToViewTree(hostLView, lContainer); diff --git a/packages/core/test/render3/view_utils_spec.ts b/packages/core/test/render3/view_utils_spec.ts index 762c932380..caedd80967 100644 --- a/packages/core/test/render3/view_utils_spec.ts +++ b/packages/core/test/render3/view_utils_spec.ts @@ -16,7 +16,7 @@ describe('view_utils', () => { const tView = createTView(TViewType.Root, 0, null, 0, 0, null, null, null, null, null); const lView = createLView(null, tView, {}, 0, div, null, {} as any, {} as any, null, null); const tNode = createTNode(null !, null, 3, 0, 'div', []); - const lContainer = createLContainer(lView, lView, div, tNode, true); + const lContainer = createLContainer(lView, lView, div, tNode); expect(isLView(lView)).toBe(true); expect(isLView(lContainer)).toBe(false);