From 1a0ee18d62b5bbe1050ed257abd7707899af9514 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Sun, 24 Nov 2019 20:56:18 -0800 Subject: [PATCH] fix(ivy): run pre-order hooks in injection order (#34026) This commit fixes a compatibility bug where pre-order lifecycle hooks (onInit, doCheck, OnChanges) for directives on the same host node were executed based on the order the directives were matched, rather than the order the directives were instantiated (i.e. injection order). This discrepancy can cause issues with forms, where it is common to inject NgControl and try to extract its control property in ngOnInit. As the NgControl directive is injected, it should be instantiated before the control value accessor directive (and thus its hooks should run first). This ensures that the NgControl ngOnInit can set up the form control before the ngOnInit for the control value accessor tries to access it. Closes #32522 PR Close #34026 --- integration/_payload-limits.json | 2 +- packages/core/src/render3/assert.ts | 11 + packages/core/src/render3/component.ts | 1 - packages/core/src/render3/di.ts | 19 +- packages/core/src/render3/di_setup.ts | 5 +- packages/core/src/render3/hooks.ts | 20 +- .../core/src/render3/instructions/shared.ts | 32 +- packages/core/src/render3/query.ts | 2 +- .../core/test/acceptance/lifecycle_spec.ts | 315 +++++++++++++++++- .../hello_world/bundle.golden_symbols.json | 3 + .../test/value_accessor_integration_spec.ts | 19 +- 11 files changed, 384 insertions(+), 45 deletions(-) diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index c2f9b2f006..1cf14f2116 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 15267, + "main-es2015": 15783, "polyfills-es2015": 36808 } } diff --git a/packages/core/src/render3/assert.ts b/packages/core/src/render3/assert.ts index 4415935875..e822917971 100644 --- a/packages/core/src/render3/assert.ts +++ b/packages/core/src/render3/assert.ts @@ -72,3 +72,14 @@ export function assertFirstCreatePass(tView: TView, errMessage?: string) { assertEqual( tView.firstCreatePass, true, errMessage || 'Should only be called in first create pass.'); } + +/** + * This is a basic sanity check that an object is probably a directive def. DirectiveDef is + * an interface, so we can't do a direct instanceof check. + */ +export function assertDirectiveDef(obj: any) { + if (obj.type === undefined || obj.selectors == undefined || obj.inputs === undefined) { + throwError( + `Expected a DirectiveDef/ComponentDef and this object does not seem to have the expected shape.`); + } +} diff --git a/packages/core/src/render3/component.ts b/packages/core/src/render3/component.ts index 4e847ecd2e..2e2825fd26 100644 --- a/packages/core/src/render3/component.ts +++ b/packages/core/src/render3/component.ts @@ -255,7 +255,6 @@ export function LifecycleHooksFeature(component: any, def: ComponentDef): v const rootTView = readPatchedLView(component) ![TVIEW]; const dirIndex = rootTView.data.length - 1; - registerPreOrderHooks(dirIndex, def, rootTView, -1, -1, -1); // TODO(misko): replace `as TNode` with createTNode call. (needs refactoring to lose dep on // LNode). registerPostOrderHooks( diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index c565dd11e5..57b8cb8058 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -15,8 +15,10 @@ import {InjectFlags} from '../di/interface/injector'; import {Type} from '../interface/type'; import {assertDefined, assertEqual} from '../util/assert'; +import {assertDirectiveDef} from './assert'; import {getFactoryDef} from './definition'; import {NG_ELEMENT_ID, NG_FACTORY_DEF} from './fields'; +import {registerPreOrderHooks} from './hooks'; import {DirectiveDef, FactoryFn} from './interfaces/definition'; import {NO_PARENT_INJECTOR, NodeInjectorFactory, PARENT_INJECTOR, RelativeInjectorLocation, RelativeInjectorLocationFlags, TNODE, isFactory} from './interfaces/injector'; import {AttributeMarker, TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TNode, TNodeProviderIndexes, TNodeType} from './interfaces/node'; @@ -471,7 +473,7 @@ function searchTokensOnInjector( const injectableIdx = locateDirectiveOrProvider( tNode, currentTView, token, canAccessViewProviders, isHostSpecialCase); if (injectableIdx !== null) { - return getNodeInjectable(currentTView.data, lView, injectableIdx, tNode as TElementNode); + return getNodeInjectable(lView, currentTView, injectableIdx, tNode as TElementNode); } else { return NOT_FOUND; } @@ -519,15 +521,16 @@ export function locateDirectiveOrProvider( } /** -* Retrieve or instantiate the injectable from the `lData` at particular `index`. +* Retrieve or instantiate the injectable from the `LView` at particular `index`. * * This function checks to see if the value has already been instantiated and if so returns the * cached `injectable`. Otherwise if it detects that the value is still a factory it * instantiates the `injectable` and caches the value. */ export function getNodeInjectable( - tData: TData, lView: LView, index: number, tNode: TDirectiveHostNode): any { + lView: LView, tView: TView, index: number, tNode: TDirectiveHostNode): any { let value = lView[index]; + const tData = tView.data; if (isFactory(value)) { const factory: NodeInjectorFactory = value; if (factory.resolving) { @@ -542,6 +545,16 @@ export function getNodeInjectable( enterDI(lView, tNode); try { value = lView[index] = factory.factory(undefined, tData, lView, tNode); + // This code path is hit for both directives and providers. + // For perf reasons, we want to avoid searching for hooks on providers. + // It does no harm to try (the hooks just won't exist), but the extra + // checks are unnecessary and this is a hot path. So we check to see + // if the index of the dependency is in the directive range for this + // tNode. If it's not, we know it's a provider and skip hook registration. + if (tView.firstCreatePass && index >= tNode.directiveStart) { + ngDevMode && assertDirectiveDef(tData[index]); + registerPreOrderHooks(index, tData[index] as DirectiveDef, tView); + } } finally { if (factory.injectImpl) setInjectImplementation(previousInjectImplementation); setIncludeViewProviders(previousIncludeViewProviders); diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index f400d40ecc..e0815cc58b 100644 --- a/packages/core/src/render3/di_setup.ts +++ b/packages/core/src/render3/di_setup.ts @@ -215,13 +215,14 @@ function multiProvidersFactoryResolver( * This factory knows how to concatenate itself with the existing `multi` `providers`. */ function multiViewProvidersFactoryResolver( - this: NodeInjectorFactory, _: undefined, tData: TData, lData: LView, + this: NodeInjectorFactory, _: undefined, tData: TData, lView: LView, tNode: TDirectiveHostNode): any[] { const factories = this.multi !; let result: any[]; if (this.providerFactory) { const componentCount = this.providerFactory.componentProviders !; - const multiProviders = getNodeInjectable(tData, lData, this.providerFactory !.index !, tNode); + const multiProviders = + getNodeInjectable(lView, lView[TVIEW], this.providerFactory !.index !, tNode); // Copy the section of the array which contains `multi` `providers` from the component result = multiProviders.slice(0, componentCount); // Insert the `viewProvider` instances. diff --git a/packages/core/src/render3/hooks.ts b/packages/core/src/render3/hooks.ts index 8bedf39135..eebaed60aa 100644 --- a/packages/core/src/render3/hooks.ts +++ b/packages/core/src/render3/hooks.ts @@ -27,29 +27,11 @@ import {getCheckNoChangesMode} from './state'; * @param directiveIndex The index of the directive in LView * @param directiveDef The definition containing the hooks to setup in tView * @param tView The current TView - * @param nodeIndex The index of the node to which the directive is attached - * @param initialPreOrderHooksLength the number of pre-order hooks already registered before the - * current process, used to know if the node index has to be added to the array. If it is -1, - * the node index is never added. - * @param initialPreOrderCheckHooksLength same as previous for pre-order check hooks */ export function registerPreOrderHooks( - directiveIndex: number, directiveDef: DirectiveDef, tView: TView, nodeIndex: number, - initialPreOrderHooksLength: number, initialPreOrderCheckHooksLength: number): void { + directiveIndex: number, directiveDef: DirectiveDef, tView: TView): void { ngDevMode && assertFirstCreatePass(tView); const {onChanges, onInit, doCheck} = directiveDef; - if (initialPreOrderHooksLength >= 0 && - (!tView.preOrderHooks || initialPreOrderHooksLength === tView.preOrderHooks.length) && - (onChanges || onInit || doCheck)) { - (tView.preOrderHooks || (tView.preOrderHooks = [])).push(nodeIndex); - } - - if (initialPreOrderCheckHooksLength >= 0 && - (!tView.preOrderCheckHooks || - initialPreOrderCheckHooksLength === tView.preOrderCheckHooks.length) && - (onChanges || doCheck)) { - (tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [])).push(nodeIndex); - } if (onChanges) { (tView.preOrderHooks || (tView.preOrderHooks = [])).push(directiveIndex, onChanges); diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index f2288a477f..f11ae30d21 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -1057,8 +1057,7 @@ export function instantiateRootComponent(tView: TView, lView: LView, def: Com generateExpandoInstructionBlock(tView, rootTNode, 1); baseResolveDirective(tView, lView, def); } - const directive = - getNodeInjectable(tView.data, lView, lView.length - 1, rootTNode as TElementNode); + const directive = getNodeInjectable(lView, tView, lView.length - 1, rootTNode as TElementNode); attachPatchData(directive, lView); const native = getNativeByTNode(rootTNode, lView); if (native) { @@ -1097,14 +1096,11 @@ export function resolveDirectives( if (def.providersResolver) def.providersResolver(def); } generateExpandoInstructionBlock(tView, tNode, directives.length); - const initialPreOrderHooksLength = (tView.preOrderHooks && tView.preOrderHooks.length) || 0; - const initialPreOrderCheckHooksLength = - (tView.preOrderCheckHooks && tView.preOrderCheckHooks.length) || 0; - const nodeIndex = tNode.index - HEADER_OFFSET; + let preOrderHooksFound = false; + let preOrderCheckHooksFound = false; for (let i = 0; i < directives.length; i++) { const def = directives[i] as DirectiveDef; - const directiveDefIdx = tView.data.length; baseResolveDirective(tView, lView, def); saveNameToExportMap(tView.data !.length - 1, def, exportsMap); @@ -1112,11 +1108,21 @@ export function resolveDirectives( if (def.contentQueries !== null) tNode.flags |= TNodeFlags.hasContentQuery; if (def.hostBindings !== null) tNode.flags |= TNodeFlags.hasHostBindings; - // Init hooks are queued now so ngOnInit is called in host components before - // any projected components. - registerPreOrderHooks( - directiveDefIdx, def, tView, nodeIndex, initialPreOrderHooksLength, - initialPreOrderCheckHooksLength); + // Only push a node index into the preOrderHooks array if this is the first + // pre-order hook found on this node. + if (!preOrderHooksFound && (def.onChanges || def.onInit || def.doCheck)) { + // We will push the actual hook function into this array later during dir instantiation. + // We cannot do it now because we must ensure hooks are registered in the same + // order that directives are created (i.e. injection order). + (tView.preOrderHooks || (tView.preOrderHooks = [])).push(tNode.index - HEADER_OFFSET); + preOrderHooksFound = true; + } + + if (!preOrderCheckHooksFound && (def.onChanges || def.doCheck)) { + (tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [ + ])).push(tNode.index - HEADER_OFFSET); + preOrderCheckHooksFound = true; + } } initializeInputAndOutputAliases(tView, tNode); @@ -1148,7 +1154,7 @@ function instantiateAllDirectives( addComponentLogic(lView, tNode as TElementNode, def as ComponentDef); } - const directive = getNodeInjectable(tView.data, lView, i, tNode); + const directive = getNodeInjectable(lView, tView, i, tNode); attachPatchData(directive, lView); if (initialInputs !== null) { diff --git a/packages/core/src/render3/query.ts b/packages/core/src/render3/query.ts index b6d43f18a0..0717eeae84 100644 --- a/packages/core/src/render3/query.ts +++ b/packages/core/src/render3/query.ts @@ -281,7 +281,7 @@ function createResultForNode(lView: LView, tNode: TNode, matchingIdx: number, re return createSpecialToken(lView, tNode, read); } else { // read a token - return getNodeInjectable(lView[TVIEW].data, lView, matchingIdx, tNode as TElementNode); + return getNodeInjectable(lView, lView[TVIEW], matchingIdx, tNode as TElementNode); } } diff --git a/packages/core/test/acceptance/lifecycle_spec.ts b/packages/core/test/acceptance/lifecycle_spec.ts index 5175d3dd7d..359be58390 100644 --- a/packages/core/test/acceptance/lifecycle_spec.ts +++ b/packages/core/test/acceptance/lifecycle_spec.ts @@ -535,7 +535,7 @@ describe('onChanges', () => { ]); }); - it('should be called on directives after component', () => { + it('should be called on directives after component by default', () => { const events: any[] = []; @Directive({ @@ -607,6 +607,138 @@ describe('onChanges', () => { ]); }); + it('should be called on directives before component if component injects directives', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'dir', changes}); } + } + + @Component({ + selector: 'comp', + template: `

{{val}}

`, + }) + class Comp { + @Input() + val = ''; + + constructor(public dir: Dir) {} + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'comp', changes}); } + } + + @Component({ + template: ``, + }) + class App { + val = 'a'; + } + + TestBed.configureTestingModule({ + declarations: [App, Comp, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual([ + { + name: 'dir', + changes: { + dir: new SimpleChange(undefined, 'a', true), + } + }, + { + name: 'comp', + changes: { + val: new SimpleChange(undefined, 'a', true), + } + } + ]); + + events.length = 0; + fixture.componentInstance.val = 'b'; + fixture.detectChanges(); + + expect(events).toEqual([ + { + name: 'dir', + changes: { + dir: new SimpleChange('a', 'b', false), + } + }, + { + name: 'comp', + changes: { + val: new SimpleChange('a', 'b', false), + } + } + ]); + + }); + + it('should be called on multiple directives in injection order', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'dir', changes}); } + } + + @Directive({ + selector: '[injectionDir]', + }) + class InjectionDir { + @Input() + injectionDir = ''; + + constructor(public dir: Dir) {} + + ngOnChanges(changes: SimpleChanges) { events.push({name: 'injectionDir', changes}); } + } + + @Component({ + template: `
`, + }) + class App { + val = 'a'; + } + + TestBed.configureTestingModule({ + declarations: [App, InjectionDir, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual([ + { + name: 'dir', + changes: { + dir: new SimpleChange(undefined, 'a', true), + } + }, + { + name: 'injectionDir', + changes: { + injectionDir: new SimpleChange(undefined, 'a', true), + } + } + ]); + }); + + it('should be called on directives on an element', () => { const events: any[] = []; @@ -1412,7 +1544,7 @@ describe('onInit', () => { expect(initialized).toEqual(['app', 'comp 1', 'projected 1', 'comp 2', 'projected 2']); }); - it('should be called on directives after component', () => { + it('should be called on directives after component by default', () => { const initialized: string[] = []; @Directive({ @@ -1455,6 +1587,95 @@ describe('onInit', () => { expect(initialized).toEqual(['app', 'comp 1', 'dir 1', 'comp 2', 'dir 2']); }); + it('should be called on multiple directives in injection order', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngOnInit() { events.push('dir'); } + } + + @Directive({ + selector: '[injectionDir]', + }) + class InjectionDir { + @Input() + injectionDir = ''; + + constructor(public dir: Dir) {} + + ngOnInit() { events.push('injectionDir'); } + } + + @Component({ + template: `
`, + }) + class App { + val = 'a'; + + ngOnInit() { events.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, InjectionDir, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual(['app', 'dir', 'injectionDir']); + }); + + it('should be called on directives before component if component injects directives', () => { + const initialized: string[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input('dir-name') + name = ''; + + ngOnInit() { initialized.push('dir ' + this.name); } + } + + @Component({ + selector: 'comp', + template: `

`, + }) + class Comp { + @Input() + name = ''; + + constructor(public dir: Dir) {} + + ngOnInit() { initialized.push('comp ' + this.name); } + } + + @Component({ + template: ` + + + ` + }) + class App { + ngOnInit() { initialized.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, Comp, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(initialized).toEqual(['app', 'dir 1', 'comp 1', 'dir 2', 'comp 2']); + }); + it('should be called on directives on an element', () => { const initialized: string[] = []; @@ -1662,7 +1883,7 @@ describe('doCheck', () => { expect(events).toEqual(['onInit', 'doCheck']); }); - it('should be called on directives after component', () => { + it('should be called on directives after component by default', () => { const doChecks: string[] = []; @Directive({ selector: '[dir]', @@ -1704,6 +1925,94 @@ describe('doCheck', () => { expect(doChecks).toEqual(['app', 'comp 1', 'dir 1', 'comp 2', 'dir 2']); }); + it('should be called on directives before component if component injects directives', () => { + const doChecks: string[] = []; + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input('dir') + name = ''; + + ngDoCheck() { doChecks.push('dir ' + this.name); } + } + + @Component({ + selector: 'comp', + template: `

test

`, + }) + class Comp { + @Input() + name = ''; + + constructor(public dir: Dir) {} + + ngDoCheck() { doChecks.push('comp ' + this.name); } + } + + @Component({ + template: ` + + + ` + }) + class App { + ngDoCheck() { doChecks.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, Comp, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(doChecks).toEqual(['app', 'dir 1', 'comp 1', 'dir 2', 'comp 2']); + }); + + it('should be called on multiple directives in injection order', () => { + + const events: any[] = []; + + @Directive({ + selector: '[dir]', + }) + class Dir { + @Input() + dir = ''; + + ngDoCheck() { events.push('dir'); } + } + + @Directive({ + selector: '[injectionDir]', + }) + class InjectionDir { + @Input() + injectionDir = ''; + + constructor(public dir: Dir) {} + + ngDoCheck() { events.push('injectionDir'); } + } + + @Component({ + template: `
`, + }) + class App { + val = 'a'; + + ngDoCheck() { events.push('app'); } + } + + TestBed.configureTestingModule({ + declarations: [App, InjectionDir, Dir], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(events).toEqual(['app', 'dir', 'injectionDir']); + }); + it('should be called on directives on an element', () => { const doChecks: string[] = []; 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 1f6c456c07..7bfaa2eccd 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -416,6 +416,9 @@ { "name": "refreshView" }, + { + "name": "registerPreOrderHooks" + }, { "name": "renderChildComponents" }, diff --git a/packages/forms/test/value_accessor_integration_spec.ts b/packages/forms/test/value_accessor_integration_spec.ts index a8061fe60c..7eac63877d 100644 --- a/packages/forms/test/value_accessor_integration_spec.ts +++ b/packages/forms/test/value_accessor_integration_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, Directive, EventEmitter, Input, Output, Type} from '@angular/core'; +import {Component, Directive, EventEmitter, Input, Output, Type, ViewChild} from '@angular/core'; import {ComponentFixture, TestBed, async, fakeAsync, tick} from '@angular/core/testing'; import {AbstractControl, ControlValueAccessor, FormControl, FormGroup, FormsModule, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, NgForm, NgModel, ReactiveFormsModule, Validators} from '@angular/forms'; import {By} from '@angular/platform-browser/src/dom/debug/by'; @@ -1055,6 +1055,16 @@ import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util' fixture.detectChanges(); expect(fixture.componentInstance.control.status).toEqual('DISABLED'); }); + + it('should populate control in ngOnInit when injecting NgControl', () => { + const fixture = initTest(MyInputForm, MyInput); + fixture.componentInstance.form = new FormGroup({'login': new FormControl('aa')}); + fixture.detectChanges(); + + expect(fixture.componentInstance.myInput !.control).toBeDefined(); + expect(fixture.componentInstance.myInput !.control) + .toEqual(fixture.componentInstance.myInput !.controlDir.control); + }); }); describe('in template-driven forms', () => { @@ -1359,7 +1369,11 @@ export class MyInput implements ControlValueAccessor { // TODO(issue/24571): remove '!'. value !: string; - constructor(cd: NgControl) { cd.valueAccessor = this; } + control: AbstractControl|null = null; + + constructor(public controlDir: NgControl) { controlDir.valueAccessor = this; } + + ngOnInit() { this.control = this.controlDir.control; } writeValue(value: any) { this.value = `!${value}!`; } @@ -1380,6 +1394,7 @@ export class MyInput implements ControlValueAccessor { export class MyInputForm { // TODO(issue/24571): remove '!'. form !: FormGroup; + @ViewChild(MyInput) myInput: MyInput|null = null; } @Component({