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

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 =
-      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,26 +1096,33 @@ 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<any>;
 
-      const directiveDefIdx = tView.data.length;
       baseResolveDirective(tView, lView, def);
 
       saveNameToExportMap(tView.data !.length - 1, def, exportsMap);
 
       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<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);
   }
 }