fix(ivy): correct timing of NgModuleFactory registration (angular#30706)

Commit 0df719a introduced registration of NgModules with ids when compiled
with AOT, and f74373f corrected the timing to avoid issues with tree
shaking. Neither of these approaches were correct.

This commit fixes the timing to match View Engine and avoid tree shaking
issues, as well as fixes a bug with the registration of imported module ids.

A new Ivy-only test is added which verifies that modules get registered
correctly under real-world conditions.

PR Close angular#30706

Author: alxhub

packages/core/src/linker/ng_module_factory_registration.ts

@@ -37,10 +37,21 @@ function assertSameOrNotExisting(id: string, type: Type<any>| null, incoming: Ty
   }
 }
 
-export function registerNgModuleType(id: string, ngModuleType: NgModuleType) {
-  const existing = modules.get(id) as NgModuleType | null;
-  assertSameOrNotExisting(id, existing, ngModuleType);
-  modules.set(id, ngModuleType);
+export function registerNgModuleType(ngModuleType: NgModuleType) {
+  if (ngModuleType.ngModuleDef.id !== null) {
+    const id = ngModuleType.ngModuleDef.id;
+    const existing = modules.get(id) as NgModuleType | null;
+    assertSameOrNotExisting(id, existing, ngModuleType);
+    modules.set(id, ngModuleType);
+  }
+
+  let imports = ngModuleType.ngModuleDef.imports;
+  if (imports instanceof Function) {
+    imports = imports();
+  }
+  if (imports) {
+    imports.forEach((i: NgModuleType<any>) => registerNgModuleType(i));
+  }
 }
 
 export function clearModulesForTest(): void {

packages/core/src/render3/ng_module_ref.ts

@@ -92,14 +92,39 @@ export class NgModuleRef<T> extends viewEngine_NgModuleRef<T> implements Interna
 }
 
 export class NgModuleFactory<T> extends viewEngine_NgModuleFactory<T> {
-  constructor(public moduleType: Type<T>) { super(); }
+  constructor(public moduleType: Type<T>) {
+    super();
 
-  create(parentInjector: Injector|null): viewEngine_NgModuleRef<T> {
-    const moduleType = this.moduleType;
-    const moduleRef = new NgModuleRef(moduleType, parentInjector);
     const ngModuleDef = getNgModuleDef(moduleType);
-    ngModuleDef && ngModuleDef.id &&
-        registerNgModuleType(ngModuleDef.id, moduleType as NgModuleType);
-    return moduleRef;
+    if (ngModuleDef !== null) {
+      // Register the NgModule with Angular's module registry. The location (and hence timing) of
+      // this call is critical to ensure this works correctly (modules get registered when expected)
+      // without bloating bundles (modules are registered when otherwise not referenced).
+      //
+      // In View Engine, registration occurs in the .ngfactory.js file as a side effect. This has
+      // several practical consequences:
+      //
+      // - If an .ngfactory file is not imported from, the module won't be registered (and can be
+      //   tree shaken).
+      // - If an .ngfactory file is imported from, the module will be registered even if an instance
+      //   is not actually created (via `create` below).
+      // - Since an .ngfactory file in View Engine references the .ngfactory files of the NgModule's
+      //   imports,
+      //
+      // In Ivy, things are a bit different. .ngfactory files still exist for compatibility, but are
+      // not a required API to use - there are other ways to obtain an NgModuleFactory for a given
+      // NgModule. Thus, relying on a side effect in the .ngfactory file is not sufficient. Instead,
+      // the side effect of registration is added here, in the constructor of NgModuleFactory,
+      // ensuring no matter how a factory is created, the module is registered correctly.
+      //
+      // An alternative would be to include the registration side effect inline following the actual
+      // NgModule definition. This also has the correct timing, but breaks tree-shaking - modules
+      // will be registered and retained even if they're otherwise never referenced.
+      registerNgModuleType(moduleType as NgModuleType);
+    }
+  }
+
+  create(parentInjector: Injector|null): viewEngine_NgModuleRef<T> {
+    return new NgModuleRef(this.moduleType, parentInjector);
   }
 }

packages/core/test/linker/ng_module_integration_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ANALYZE_FOR_ENTRY_COMPONENTS, CUSTOM_ELEMENTS_SCHEMA, ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, Directive, HostBinding, Inject, Injectable, InjectionToken, Injector, Input, NgModule, NgModuleRef, Optional, Pipe, Provider, Self, Type, forwardRef, getModuleFactory, ɵivyEnabled as ivyEnabled} from '@angular/core';
+import {ANALYZE_FOR_ENTRY_COMPONENTS, CUSTOM_ELEMENTS_SCHEMA, ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, Directive, HostBinding, Inject, Injectable, InjectionToken, Injector, Input, NgModule, NgModuleRef, Optional, Pipe, Provider, Self, Type, forwardRef, getModuleFactory, ɵivyEnabled as ivyEnabled, ɵɵdefineNgModule as defineNgModule} from '@angular/core';
 import {Console} from '@angular/core/src/console';
 import {ɵɵInjectableDef, ɵɵdefineInjectable} from '@angular/core/src/di/interface/defs';
 import {getNgModuleDef} from '@angular/core/src/render3/definition';
@@ -339,6 +339,28 @@ function declareTests(config?: {useJit: boolean}) {
           }
         }).not.toThrow();
       });
+
+      onlyInIvy('VE does not allow use of NgModuleFactory without importing the .ngfactory')
+          .it('should register a module even if not importing the .ngfactory file or calling create()',
+              () => {
+                class ChildModule {
+                  static ngModuleDef = defineNgModule({
+                    type: ChildModule,
+                    id: 'child',
+                  });
+                }
+
+                class Module {
+                  static ngModuleDef = defineNgModule({
+                    type: Module,
+                    id: 'test',
+                    imports: [ChildModule],
+                  });
+                }
+
+                createModuleFactory(ChildModule);
+                expect(getModuleFactory('child')).toBeAnInstanceOf(NgModuleFactory);
+              });
     });
 
     describe('entryComponents', () => {