fix(ivy): NgOnChangesFeature no longer included in hello_world (#28187)

- Wraps the NgOnChangesFeature in a factory such that no side effects occur in the module root
- Adds comments to ngInherit property on feature definition interface to help guide others not to make the same mistake
- Updates compiler to generate the feature properly after the change to it being a factory
- Updates appropriate tests

PR Close #28187

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

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

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

@@ -125,7 +125,7 @@ function baseDirectiveFields(
  */
 function addFeatures(
     definitionMap: DefinitionMap, meta: R3DirectiveMetadata | R3ComponentMetadata) {
-  // e.g. `features: [NgOnChangesFeature]`
+  // e.g. `features: [NgOnChangesFeature()]`
   const features: o.Expression[] = [];
 
   const providers = meta.providers;
@@ -142,7 +142,7 @@ function addFeatures(
     features.push(o.importExpr(R3.InheritDefinitionFeature));
   }
   if (meta.lifecycle.usesOnChanges) {
-    features.push(o.importExpr(R3.NgOnChangesFeature));
+    features.push(o.importExpr(R3.NgOnChangesFeature).callFn(EMPTY_ARRAY));
   }
   if (features.length) {
     definitionMap.set('features', o.literalArr(features));

packages/core/src/render3/features/ng_onchanges_feature.ts

@@ -35,11 +35,18 @@ type OnChangesExpando = OnChanges & {
  * static ngComponentDef = defineComponent({
  *   ...
  *   inputs: {name: 'publicName'},
- *   features: [NgOnChangesFeature]
+ *   features: [NgOnChangesFeature()]
  * });
  * ```
  */
-export function NgOnChangesFeature<T>(definition: DirectiveDef<T>): void {
+export function NgOnChangesFeature<T>(): DirectiveDefFeature {
+  // This option ensures that the ngOnChanges lifecycle hook will be inherited
+  // from superclasses (in InheritDefinitionFeature).
+  (NgOnChangesFeatureImpl as DirectiveDefFeature).ngInherit = true;
+  return NgOnChangesFeatureImpl;
+}
+
+function NgOnChangesFeatureImpl<T>(definition: DirectiveDef<T>): void {
   if (definition.type.prototype.ngOnChanges) {
     definition.setInput = ngOnChangesSetInput;
 
@@ -91,11 +98,6 @@ function setSimpleChangesStore(instance: any, store: NgSimpleChangesStore): NgSi
   return instance[SIMPLE_CHANGES_STORE] = store;
 }
 
-// This option ensures that the ngOnChanges lifecycle hook will be inherited
-// from superclasses (in InheritDefinitionFeature).
-(NgOnChangesFeature as DirectiveDefFeature).ngInherit = true;
-
-
 interface NgSimpleChangesStore {
   previous: SimpleChanges;
   current: SimpleChanges|null;

packages/core/src/render3/interfaces/definition.ts

@@ -302,11 +302,27 @@ export type PipeDefWithMeta<T, Name extends string> = PipeDef<T>;
 
 export interface DirectiveDefFeature {
   <T>(directiveDef: DirectiveDef<T>): void;
+  /**
+   * Marks a feature as something that {@link InheritDefinitionFeature} will execute
+   * during inheritance.
+   *
+   * NOTE: DO NOT SET IN ROOT OF MODULE! Doing so will result in tree-shakers/bundlers
+   * identifying the change as a side effect, and the feature will be included in
+   * every bundle.
+   */
   ngInherit?: true;
 }
 
 export interface ComponentDefFeature {
   <T>(componentDef: ComponentDef<T>): void;
+  /**
+   * Marks a feature as something that {@link InheritDefinitionFeature} will execute
+   * during inheritance.
+   *
+   * NOTE: DO NOT SET IN ROOT OF MODULE! Doing so will result in tree-shakers/bundlers
+   * identifying the change as a side effect, and the feature will be included in
+   * every bundle.
+   */
   ngInherit?: true;
 }
 

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

@@ -80,9 +80,6 @@
   {
     "name": "NO_PARENT_INJECTOR"
   },
-  {
-    "name": "NgOnChangesFeature"
-  },
   {
     "name": "NodeInjectorFactory"
   },
@@ -104,12 +101,6 @@
   {
     "name": "SANITIZER"
   },
-  {
-    "name": "SIMPLE_CHANGES_STORE"
-  },
-  {
-    "name": "SimpleChange"
-  },
   {
     "name": "TVIEW"
   },
@@ -308,9 +299,6 @@
   {
     "name": "getRootView"
   },
-  {
-    "name": "getSimpleChangesStore"
-  },
   {
     "name": "hasParentInjector"
   },
@@ -368,9 +356,6 @@
   {
     "name": "nextNgElementId"
   },
-  {
-    "name": "ngOnChangesSetInput"
-  },
   {
     "name": "noSideEffects"
   },
@@ -440,9 +425,6 @@
   {
     "name": "setPreviousOrParentTNode"
   },
-  {
-    "name": "setSimpleChangesStore"
-  },
   {
     "name": "setTNodeAndViewData"
   },
@@ -460,8 +442,5 @@
   },
   {
     "name": "viewAttached"
-  },
-  {
-    "name": "wrapOnChanges"
   }
 ]

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

@@ -8,9 +8,6 @@
   {
     "name": "EMPTY_ARRAY"
   },
-  {
-    "name": "EMPTY_OBJ"
-  },
   {
     "name": "EmptyErrorImpl"
   },
@@ -38,9 +35,6 @@
   {
     "name": "NULL_INJECTOR"
   },
-  {
-    "name": "NgOnChangesFeature"
-  },
   {
     "name": "NullInjector"
   },
@@ -56,18 +50,12 @@
   {
     "name": "R3Injector"
   },
-  {
-    "name": "SIMPLE_CHANGES_STORE"
-  },
   {
     "name": "ScopedService"
   },
   {
     "name": "Self"
   },
-  {
-    "name": "SimpleChange"
-  },
   {
     "name": "SkipSelf"
   },
@@ -125,9 +113,6 @@
   {
     "name": "getNullInjector"
   },
-  {
-    "name": "getSimpleChangesStore"
-  },
   {
     "name": "hasDeps"
   },
@@ -170,9 +155,6 @@
   {
     "name": "makeRecord"
   },
-  {
-    "name": "ngOnChangesSetInput"
-  },
   {
     "name": "providerToFactory"
   },
@@ -185,13 +167,7 @@
   {
     "name": "setCurrentInjector"
   },
-  {
-    "name": "setSimpleChangesStore"
-  },
   {
     "name": "stringify"
-  },
-  {
-    "name": "wrapOnChanges"
   }
 ]

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

@@ -143,9 +143,6 @@
   {
     "name": "NgModuleRef"
   },
-  {
-    "name": "NgOnChangesFeature"
-  },
   {
     "name": "NodeInjector"
   },
@@ -185,9 +182,6 @@
   {
     "name": "SANITIZER"
   },
-  {
-    "name": "SIMPLE_CHANGES_STORE"
-  },
   {
     "name": "SWITCH_ELEMENT_REF_FACTORY"
   },
@@ -197,9 +191,6 @@
   {
     "name": "SWITCH_VIEW_CONTAINER_REF_FACTORY"
   },
-  {
-    "name": "SimpleChange"
-  },
   {
     "name": "SkipSelf"
   },
@@ -785,9 +776,6 @@
   {
     "name": "getRootView"
   },
-  {
-    "name": "getSimpleChangesStore"
-  },
   {
     "name": "getSinglePropIndexValue"
   },
@@ -1016,9 +1004,6 @@
   {
     "name": "nextNgElementId"
   },
-  {
-    "name": "ngOnChangesSetInput"
-  },
   {
     "name": "noSideEffects"
   },
@@ -1187,9 +1172,6 @@
   {
     "name": "setSanitizeFlag"
   },
-  {
-    "name": "setSimpleChangesStore"
-  },
   {
     "name": "setStyle"
   },
@@ -1255,8 +1237,5 @@
   },
   {
     "name": "wrapListenerWithPreventDefault"
-  },
-  {
-    "name": "wrapOnChanges"
   }
 ]

packages/core/test/render3/common_with_def.ts

@@ -40,7 +40,7 @@ NgForOf.ngDirectiveDef = defineDirective({
   type: NgTemplateOutletDef,
   selectors: [['', 'ngTemplateOutlet', '']],
   factory: () => new NgTemplateOutletDef(directiveInject(ViewContainerRef as any)),
-  features: [NgOnChangesFeature],
+  features: [NgOnChangesFeature()],
   inputs:
       {ngTemplateOutlet: 'ngTemplateOutlet', ngTemplateOutletContext: 'ngTemplateOutletContext'}
 });

packages/core/test/render3/host_binding_spec.ts

@@ -357,7 +357,7 @@ describe('host bindings', () => {
         template: (rf: RenderFlags, ctx: InitHookComp) => {},
         consts: 0,
         vars: 0,
-        features: [NgOnChangesFeature],
+        features: [NgOnChangesFeature()],
         hostBindings: (rf: RenderFlags, ctx: InitHookComp, elIndex: number) => {
           if (rf & RenderFlags.Create) {
             allocHostVars(1);

packages/core/test/render3/inherit_definition_feature_spec.ts

@@ -501,7 +501,7 @@ describe('InheritDefinitionFeature', () => {
         type: SuperDirective,
         selectors: [['', 'superDir', '']],
         factory: () => new SuperDirective(),
-        features: [NgOnChangesFeature],
+        features: [NgOnChangesFeature()],
         inputs: {someInput: 'someInput'}
       });
     }

packages/core/test/render3/lifecycle_spec.ts

@@ -2008,7 +2008,7 @@ describe('lifecycles', () => {
           vars: vars,
           inputs: {a: 'val1', b: ['publicVal2', 'val2']}, template,
           directives: directives,
-          features: [NgOnChangesFeature],
+          features: [NgOnChangesFeature()],
         });
       };
     }
@@ -2027,7 +2027,7 @@ describe('lifecycles', () => {
         selectors: [['', 'dir', '']],
         factory: () => new Directive(),
         inputs: {a: 'val1', b: ['publicVal2', 'val2']},
-        features: [NgOnChangesFeature],
+        features: [NgOnChangesFeature()],
       });
     }
 
@@ -2732,7 +2732,7 @@ describe('lifecycles', () => {
           vars: vars,
           inputs: {val: 'val'}, template,
           directives: directives,
-          features: [NgOnChangesFeature],
+          features: [NgOnChangesFeature()],
         });
       };
     }

packages/core/test/render3/view_container_ref_spec.ts

@@ -1631,7 +1631,7 @@ describe('ViewContainerRef', () => {
             textBinding(0, interpolation1('', cmp.name, ''));
           }
         },
-        features: [NgOnChangesFeature],
+        features: [NgOnChangesFeature()],
         inputs: {name: 'name'}
       });
     }
@@ -1678,7 +1678,7 @@ describe('ViewContainerRef', () => {
             }
           },
           directives: [ComponentWithHooks, DirectiveWithVCRef],
-          features: [NgOnChangesFeature],
+          features: [NgOnChangesFeature()],
         });
       }
 
@@ -1771,7 +1771,7 @@ describe('ViewContainerRef', () => {
             }
           },
           directives: [ComponentWithHooks, DirectiveWithVCRef],
-          features: [NgOnChangesFeature],
+          features: [NgOnChangesFeature()],
         });
       }
 

tools/public_api_guard/common/common.d.ts

@@ -306,8 +306,8 @@ export declare class NgSwitchDefault {
 }
 
 export declare class NgTemplateOutlet implements OnChanges {
-    ngTemplateOutlet: TemplateRef<any>;
+    ngTemplateOutlet: TemplateRef<any> | null;
-    ngTemplateOutletContext: Object;
+    ngTemplateOutletContext: Object | null;
     constructor(_viewContainerRef: ViewContainerRef);
     ngOnChanges(changes: SimpleChanges): void;
 }