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

@@ -12,7 +12,7 @@
     "master": {
       "uncompressed": {
         "runtime-es2015": 1485,
-        "main-es2015": 15267,
+        "main-es2015": 15783,
         "polyfills-es2015": 36808
       }
     }

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.`);
+  }
+}

packages/core/src/render3/component.ts

@@ -255,7 +255,6 @@ export function LifecycleHooksFeature(component: any, def: ComponentDef<any>): 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(

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<T>(
   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<T>(
 }
 
 /**
-* 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<any>, tView);
+      }
     } finally {
       if (factory.injectImpl) setInjectImplementation(previousInjectImplementation);
       setIncludeViewProviders(previousIncludeViewProviders);

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.

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<any>, tView: TView, nodeIndex: number,
+    directiveIndex: number, directiveDef: DirectiveDef<any>, tView: TView): void {
-    initialPreOrderHooksLength: number, initialPreOrderCheckHooksLength: number): 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);

packages/core/src/render3/instructions/shared.ts

@@ -1057,8 +1057,7 @@ export function instantiateRootComponent<T>(tView: TView, lView: LView, def: Com
     generateExpandoInstructionBlock(tView, rootTNode, 1);
     baseResolveDirective(tView, lView, def);
   }
-  const directive =
+  const directive = getNodeInjectable(lView, tView, lView.length - 1, rootTNode as TElementNode);
-      getNodeInjectable(tView.data, lView, 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;
+    let preOrderHooksFound = false;
-    const initialPreOrderCheckHooksLength =
+    let preOrderCheckHooksFound = false;
-        (tView.preOrderCheckHooks && tView.preOrderCheckHooks.length) || 0;
-    const nodeIndex = tNode.index - HEADER_OFFSET;
     for (let i = 0; i < directives.length; i++) {
       const def = directives[i] as DirectiveDef<any>;
 
-      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
+      // Only push a node index into the preOrderHooks array if this is the first
-      // any projected components.
+      // pre-order hook found on this node.
-      registerPreOrderHooks(
+      if (!preOrderHooksFound && (def.onChanges || def.onInit || def.doCheck)) {
-          directiveDefIdx, def, tView, nodeIndex, initialPreOrderHooksLength,
+        // We will push the actual hook function into this array later during dir instantiation.
-          initialPreOrderCheckHooksLength);
+        // 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<any>);
     }
 
-    const directive = getNodeInjectable(tView.data, lView, i, tNode);
+    const directive = getNodeInjectable(lView, tView, i, tNode);
     attachPatchData(directive, lView);
 
     if (initialInputs !== null) {

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);
   }
 }
 

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: `<p>{{val}}</p>`,
+    })
+    class Comp {
+      @Input()
+      val = '';
+
+      constructor(public dir: Dir) {}
+
+      ngOnChanges(changes: SimpleChanges) { events.push({name: 'comp', changes}); }
+    }
+
+    @Component({
+      template: `<comp [dir]="val" [val]="val"></comp>`,
+    })
+    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: `<div [injectionDir]="val" [dir]="val"></div>`,
+    })
+    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: `<div [injectionDir]="val" [dir]="val"></div>`,
+    })
+    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: `<p></p>`,
+    })
+    class Comp {
+      @Input()
+      name = '';
+
+      constructor(public dir: Dir) {}
+
+      ngOnInit() { initialized.push('comp ' + this.name); }
+    }
+
+    @Component({
+      template: `
+        <comp name="1" dir dir-name="1"></comp>
+        <comp name="2" dir dir-name="2"></comp>
+      `
+    })
+    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: `<p>test</p>`,
+    })
+    class Comp {
+      @Input()
+      name = '';
+
+      constructor(public dir: Dir) {}
+
+      ngDoCheck() { doChecks.push('comp ' + this.name); }
+    }
+
+    @Component({
+      template: `
+      <comp name="1" dir="1"></comp>
+      <comp name="2" dir="2"></comp>
+    `
+    })
+    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: `<div [injectionDir]="val" [dir]="val"></div>`,
+    })
+    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[] = [];
 

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -416,6 +416,9 @@
   {
     "name": "refreshView"
   },
+  {
+    "name": "registerPreOrderHooks"
+  },
   {
     "name": "renderChildComponents"
   },

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({