fix(ivy): ngOnChanges only runs for binding updates (angular#27965)

PR Close angular#27965

Author: benlesh

integration/_payload-limits.json

@@ -3,7 +3,7 @@
     "master": {
       "uncompressed": {
         "runtime": 1497,
-        "main": 187134,
+        "main": 187437,
         "polyfills": 59608
       }
     }

packages/common/src/directives/ng_template_outlet.ts

@@ -34,14 +34,20 @@ import {Directive, EmbeddedViewRef, Input, OnChanges, SimpleChange, SimpleChange
  */
 @Directive({selector: '[ngTemplateOutlet]'})
 export class NgTemplateOutlet implements OnChanges {
-  // TODO(issue/24571): remove '!'.
-  private _viewRef !: EmbeddedViewRef<any>;
+  private _viewRef: EmbeddedViewRef<any>|null = null;
 
-  // TODO(issue/24571): remove '!'.
-  @Input() public ngTemplateOutletContext !: Object;
+  /**
+   * A context object to attach to the {@link EmbeddedViewRef}. This should be an
+   * object, the object's keys will be available for binding by the local template `let`
+   * declarations.
+   * Using the key `$implicit` in the context object will set its value as default.
+   */
+  @Input() public ngTemplateOutletContext: Object|null = null;
 
-  // TODO(issue/24571): remove '!'.
-  @Input() public ngTemplateOutlet !: TemplateRef<any>;
+  /**
+   * A string defining the template reference and optionally the context object for the template.
+   */
+  @Input() public ngTemplateOutlet: TemplateRef<any>|null = null;
 
   constructor(private _viewContainerRef: ViewContainerRef) {}
 
@@ -97,7 +103,7 @@ export class NgTemplateOutlet implements OnChanges {
 
   private _updateExistingContext(ctx: Object): void {
     for (let propName of Object.keys(ctx)) {
-      (<any>this._viewRef.context)[propName] = (<any>this.ngTemplateOutletContext)[propName];
+      (<any>this._viewRef !.context)[propName] = (<any>this.ngTemplateOutletContext)[propName];
     }
   }
 }

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

@@ -174,11 +174,6 @@ export function extractDirectiveMetadata(
   const providers: Expression|null =
       directive.has('providers') ? new WrappedNodeExpr(directive.get('providers') !) : null;
 
-  // Determine if `ngOnChanges` is a lifecycle hook defined on the component.
-  const usesOnChanges = members.some(
-      member => !member.isStatic && member.kind === ClassMemberKind.Method &&
-          member.name === 'ngOnChanges');
-
   // Parse exportAs.
   let exportAs: string[]|null = null;
   if (directive.has('exportAs')) {
@@ -197,9 +192,6 @@ export function extractDirectiveMetadata(
   const metadata: R3DirectiveMetadata = {
     name: clazz.name !.text,
     deps: getConstructorDependencies(clazz, reflector, isCore), host,
-    lifecycle: {
-        usesOnChanges,
-    },
     inputs: {...inputsFromMeta, ...inputsFromFields},
     outputs: {...outputsFromMeta, ...outputsFromFields}, queries, selector,
     type: new WrappedNodeExpr(clazz.name !),

packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts

@@ -2117,7 +2117,6 @@ describe('compiler compliance', () => {
             selectors: [["lifecycle-comp"]],
             factory: function LifecycleComp_Factory(t) { return new (t || LifecycleComp)(); },
             inputs: {nameMin: ["name", "nameMin"]},
-            features: [$r3$.ɵNgOnChangesFeature],
             consts: 0,
             vars: 0,
             template:  function LifecycleComp_Template(rf, ctx) {},
@@ -2242,7 +2241,6 @@ describe('compiler compliance', () => {
                 factory: function ForOfDirective_Factory(t) {
                   return new (t || ForOfDirective)($r3$.ɵdirectiveInject(ViewContainerRef), $r3$.ɵdirectiveInject(TemplateRef));
                 },
-                features: [$r3$.ɵNgOnChangesFeature],
                 inputs: {forOf: "forOf"}
               });
             `;
@@ -2318,7 +2316,6 @@ describe('compiler compliance', () => {
             factory: function ForOfDirective_Factory(t) {
               return new (t || ForOfDirective)($r3$.ɵdirectiveInject(ViewContainerRef), $r3$.ɵdirectiveInject(TemplateRef));
             },
-            features: [$r3$.ɵNgOnChangesFeature],
             inputs: {forOf: "forOf"}
           });
         `;

packages/compiler/src/compiler_facade_interface.ts

@@ -115,7 +115,6 @@ export interface R3DirectiveMetadataFacade {
   queries: R3QueryMetadataFacade[];
   host: {[key: string]: string};
   propMetadata: {[key: string]: any[]};
-  lifecycle: {usesOnChanges: boolean;};
   inputs: string[];
   outputs: string[];
   usesInheritance: boolean;

packages/compiler/src/render3/r3_identifiers.ts

@@ -185,8 +185,6 @@ export class Identifiers {
   static registerContentQuery:
       o.ExternalReference = {name: 'ɵregisterContentQuery', moduleName: CORE};
 
-  static NgOnChangesFeature: o.ExternalReference = {name: 'ɵNgOnChangesFeature', moduleName: CORE};
-
   static InheritDefinitionFeature:
       o.ExternalReference = {name: 'ɵInheritDefinitionFeature', moduleName: CORE};
 

packages/compiler/src/render3/view/api.ts

@@ -74,17 +74,6 @@ export interface R3DirectiveMetadata {
     properties: {[key: string]: string};
   };
 
-  /**
-   * Information about usage of specific lifecycle events which require special treatment in the
-   * code generator.
-   */
-  lifecycle: {
-    /**
-     * Whether the directive uses NgOnChanges.
-     */
-    usesOnChanges: boolean;
-  };
-
   /**
    * A mapping of input field names to the property names.
    */

packages/compiler/src/render3/view/compiler.ts

@@ -128,7 +128,6 @@ function baseDirectiveFields(
  */
 function addFeatures(
     definitionMap: DefinitionMap, meta: R3DirectiveMetadata | R3ComponentMetadata) {
-  // e.g. `features: [NgOnChangesFeature]`
   const features: o.Expression[] = [];
 
   const providers = meta.providers;
@@ -144,9 +143,7 @@ function addFeatures(
   if (meta.usesInheritance) {
     features.push(o.importExpr(R3.InheritDefinitionFeature));
   }
-  if (meta.lifecycle.usesOnChanges) {
-    features.push(o.importExpr(R3.NgOnChangesFeature));
-  }
+
   if (features.length) {
     definitionMap.set('features', o.literalArr(features));
   }
@@ -427,10 +424,6 @@ function directiveMetadataFromGlobalMetadata(
     selector: directive.selector,
     deps: dependenciesFromGlobalMetadata(directive.type, outputCtx, reflector),
     queries: queriesFromGlobalMetadata(directive.queries, outputCtx),
-    lifecycle: {
-      usesOnChanges:
-          directive.type.lifecycleHooks.some(lifecycle => lifecycle == LifecycleHooks.OnChanges),
-    },
     host: {
       attributes: directive.hostAttributes,
       listeners: summary.hostListeners,

packages/core/src/change_detection/change_detection.ts

@@ -11,8 +11,7 @@ import {DefaultKeyValueDifferFactory} from './differs/default_keyvalue_differ';
 import {IterableDifferFactory, IterableDiffers} from './differs/iterable_differs';
 import {KeyValueDifferFactory, KeyValueDiffers} from './differs/keyvalue_differs';
 
-export {SimpleChanges} from '../metadata/lifecycle_hooks';
-export {SimpleChange, WrappedValue, devModeEqual} from './change_detection_util';
+export {WrappedValue, devModeEqual} from './change_detection_util';
 export {ChangeDetectorRef} from './change_detector_ref';
 export {ChangeDetectionStrategy, ChangeDetectorStatus, isDefaultChangeDetectionStrategy} from './constants';
 export {DefaultIterableDifferFactory} from './differs/default_iterable_differ';
@@ -21,6 +20,7 @@ export {DefaultKeyValueDifferFactory} from './differs/default_keyvalue_differ';
 export {CollectionChangeRecord, IterableChangeRecord, IterableChanges, IterableDiffer, IterableDifferFactory, IterableDiffers, NgIterable, TrackByFunction} from './differs/iterable_differs';
 export {KeyValueChangeRecord, KeyValueChanges, KeyValueDiffer, KeyValueDifferFactory, KeyValueDiffers} from './differs/keyvalue_differs';
 export {PipeTransform} from './pipe_transform';
+export {SimpleChange, SimpleChanges} from './simple_change';
 
 
 

packages/core/src/change_detection/change_detection_util.ts

@@ -64,20 +64,6 @@ export class WrappedValue {
   static isWrapped(value: any): value is WrappedValue { return value instanceof WrappedValue; }
 }
 
-/**
- * Represents a basic change from a previous to a new value.
- *
- * @publicApi
- */
-export class SimpleChange {
-  constructor(public previousValue: any, public currentValue: any, public firstChange: boolean) {}
-
-  /**
-   * Check whether the new value is the first value assigned.
-   */
-  isFirstChange(): boolean { return this.firstChange; }
-}
-
 export function isListLikeIterable(obj: any): boolean {
   if (!isJsObject(obj)) return false;
   return Array.isArray(obj) ||

packages/core/src/change_detection/simple_change.ts

@@ -0,0 +1,35 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+/**
+ * Represents a basic change from a previous to a new value for a single
+ * property on a directive instance. Passed as a value in a
+ * {@link SimpleChanges} object to the `ngOnChanges` hook.
+ *
+ * @see `OnChanges`
+ *
+ * @publicApi
+ */
+export class SimpleChange {
+  constructor(public previousValue: any, public currentValue: any, public firstChange: boolean) {}
+  /**
+   * Check whether the new value is the first value assigned.
+   */
+  isFirstChange(): boolean { return this.firstChange; }
+}
+
+/**
+ * A hashtable of changes represented by {@link SimpleChange} objects stored
+ * at the declared property name they belong to on a Directive or Component. This is
+ * the type passed to the `ngOnChanges` hook.
+ *
+ * @see `OnChanges`
+ *
+ * @publicApi
+ */
+export interface SimpleChanges { [propName: string]: SimpleChange; }

packages/core/src/core_render3_private_export.ts

@@ -28,7 +28,6 @@ export {
   templateRefExtractor as ɵtemplateRefExtractor,
   ProvidersFeature as ɵProvidersFeature,
   InheritDefinitionFeature as ɵInheritDefinitionFeature,
-  NgOnChangesFeature as ɵNgOnChangesFeature,
   LifecycleHooksFeature as ɵLifecycleHooksFeature,
   NgModuleType as ɵNgModuleType,
   NgModuleRef as ɵRender3NgModuleRef,

packages/core/src/metadata/directives.ts

@@ -9,13 +9,12 @@
 import {ChangeDetectionStrategy} from '../change_detection/constants';
 import {Provider} from '../di';
 import {Type} from '../interface/type';
-import {NG_BASE_DEF} from '../render3/fields';
+import {NG_BASE_DEF, NG_COMPONENT_DEF, NG_DIRECTIVE_DEF} from '../render3/fields';
 import {compileComponent as render3CompileComponent, compileDirective as render3CompileDirective} from '../render3/jit/directive';
 import {compilePipe as render3CompilePipe} from '../render3/jit/pipe';
 import {TypeDecorator, makeDecorator, makePropDecorator} from '../util/decorators';
 import {noop} from '../util/noop';
 import {fillProperties} from '../util/property';
-
 import {ViewEncapsulation} from './view';
 
 
@@ -715,29 +714,54 @@ const initializeBaseDef = (target: any): void => {
 };
 
 /**
- * Does the work of creating the `ngBaseDef` property for the @Input and @Output decorators.
- * @param key "inputs" or "outputs"
+ * Returns a function that will update the static definition on a class to have the
+ * appropriate input or output mapping.
+ *
+ * Will also add an {@link ngBaseDef} property to a directive if no `ngDirectiveDef`
+ * or `ngComponentDef` is present. This is done because a class may have {@link InputDecorator}s and
+ * {@link OutputDecorator}s without having a {@link ComponentDecorator} or {@link DirectiveDecorator},
+ * and those inputs and outputs should still be inheritable, we need to add an
+ * `ngBaseDef` property if there are no existing `ngComponentDef` or `ngDirectiveDef`
+ * properties, so that we can track the inputs and outputs for inheritance purposes.
+ *
+ * @param getPropertyToUpdate A function that maps to either the `inputs` property or the
+ * `outputs` property of a definition.
+ * @returns A function that, the called, will add a `ngBaseDef` if no other definition is present,
+ * then update the `inputs` or `outputs` on it, depending on what was selected by `getPropertyToUpdate`
+ *
+ *
+ * @see InputDecorator
+ * @see OutputDecorator
+ * @see InheritenceFeature
  */
-const updateBaseDefFromIOProp = (getProp: (baseDef: {inputs?: any, outputs?: any}) => any) =>
-    (target: any, name: string, ...args: any[]) => {
-      const constructor = target.constructor;
-
-      if (!constructor.hasOwnProperty(NG_BASE_DEF)) {
-        initializeBaseDef(target);
-      }
-
-      const baseDef = constructor.ngBaseDef;
-      const defProp = getProp(baseDef);
+function getOrCreateDefinitionAndUpdateMappingFor(
+    getPropertyToUpdate: (baseDef: {inputs?: any, outputs?: any}) => any) {
+  return function updateIOProp(target: any, name: string, ...args: any[]) {
+    const constructor = target.constructor;
+
+    let def: any =
+        constructor[NG_COMPONENT_DEF] || constructor[NG_DIRECTIVE_DEF] || constructor[NG_BASE_DEF];
+
+    if (!def) {
+      initializeBaseDef(target);
+      def = constructor[NG_BASE_DEF];
+    }
+
+    const defProp = getPropertyToUpdate(def);
+    // Use of `in` because we *do* want to check the prototype chain here.
+    if (!(name in defProp)) {
       defProp[name] = args[0];
-    };
+    }
+  };
+}
 
 /**
  * @Annotation
  * @publicApi
  */
 export const Input: InputDecorator = makePropDecorator(
     'Input', (bindingPropertyName?: string) => ({bindingPropertyName}), undefined,
-    updateBaseDefFromIOProp(baseDef => baseDef.inputs || {}));
+    getOrCreateDefinitionAndUpdateMappingFor(def => def.inputs || {}));
 
 /**
  * Type of the Output decorator / constructor function.
@@ -777,7 +801,7 @@ export interface Output { bindingPropertyName?: string; }
  */
 export const Output: OutputDecorator = makePropDecorator(
     'Output', (bindingPropertyName?: string) => ({bindingPropertyName}), undefined,
-    updateBaseDefFromIOProp(baseDef => baseDef.outputs || {}));
+    getOrCreateDefinitionAndUpdateMappingFor(def => def.outputs || {}));
 
 
 

packages/core/src/metadata/lifecycle_hooks.ts

@@ -5,19 +5,8 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
+import {SimpleChanges} from '../change_detection/simple_change';
 
-import {SimpleChange} from '../change_detection/change_detection_util';
-
-
-/**
- * Defines an object that associates properties with
- * instances of `SimpleChange`.
- *
- * @see `OnChanges`
- *
- * @publicApi
- */
-export interface SimpleChanges { [propName: string]: SimpleChange; }
 
 /**
  * @description

packages/core/src/render3/component.ts

@@ -17,7 +17,7 @@ import {assertComponentType} from './assert';
 import {getComponentDef} from './definition';
 import {diPublicInInjector, getOrCreateNodeInjectorForNode} from './di';
 import {publishDefaultGlobalUtils} from './global_utils';
-import {queueInitHooks, queueLifecycleHooks} from './hooks';
+import {registerPostOrderHooks, registerPreOrderHooks} from './hooks';
 import {CLEAN_PROMISE, createLView, createNodeAtIndex, createTNode, createTView, getOrCreateTView, initNodeFlags, instantiateRootComponent, locateHostElement, queueComponentIndexForCheck, refreshDescendantViews} from './instructions';
 import {ComponentDef, ComponentType, RenderFlags} from './interfaces/definition';
 import {TElementNode, TNode, TNodeFlags, TNodeType} from './interfaces/node';
@@ -237,10 +237,11 @@ export function LifecycleHooksFeature(component: any, def: ComponentDef<any>): v
   const rootTView = readPatchedLView(component) ![TVIEW];
   const dirIndex = rootTView.data.length - 1;
 
-  queueInitHooks(dirIndex, def.onInit, def.doCheck, rootTView);
+  registerPreOrderHooks(dirIndex, def, rootTView);
   // TODO(misko): replace `as TNode` with createTNode call. (needs refactoring to lose dep on
   // LNode).
-  queueLifecycleHooks(rootTView, { directiveStart: dirIndex, directiveEnd: dirIndex + 1 } as TNode);
+  registerPostOrderHooks(
+      rootTView, { directiveStart: dirIndex, directiveEnd: dirIndex + 1 } as TNode);
 }
 
 /**

packages/core/src/render3/context_discovery.ts

@@ -12,6 +12,7 @@ import {LContext, MONKEY_PATCH_KEY_NAME} from './interfaces/context';
 import {TNode, TNodeFlags} from './interfaces/node';
 import {RElement} from './interfaces/renderer';
 import {CONTEXT, HEADER_OFFSET, HOST, LView, TVIEW} from './interfaces/view';
+import {unwrapOnChangesDirectiveWrapper} from './onchanges_util';
 import {getComponentViewByIndex, getNativeByTNode, readElementValue, readPatchedData} from './util';
 
 
@@ -257,7 +258,7 @@ function findViaDirective(lView: LView, directiveInstance: {}): number {
     const directiveIndexStart = tNode.directiveStart;
     const directiveIndexEnd = tNode.directiveEnd;
     for (let i = directiveIndexStart; i < directiveIndexEnd; i++) {
-      if (lView[i] === directiveInstance) {
+      if (unwrapOnChangesDirectiveWrapper(lView[i]) === directiveInstance) {
         return tNode.index;
       }
     }