fix(ivy): align NgModule registration timing with ViewEngine (#30244)

Currently in Ivy `NgModule` registration happens when the class is declared, however this is inconsistent with ViewEngine and requires extra generated code. These changes remove the generated code for `registerModuleFactory`, pass the id through to the `ngModuleDef` and do the module registration inside `NgModuleFactory.create`.

This PR resolves FW-1285.

PR Close #30244

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

@@ -162,6 +162,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
       exports,
       imports,
       containsForwardDecls,
+      id,
       emitInline: false,
       // TODO: to be implemented as a part of FW-1004.
       schemas: [],
@@ -257,11 +258,6 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
         ngModuleStatements.push(callExpr.toStmt());
       }
     }
-    if (analysis.id !== null) {
-      const registerNgModuleType = new ExternalExpr(R3Identifiers.registerNgModuleType);
-      const callExpr = registerNgModuleType.callFn([analysis.id, new WrappedNodeExpr(node.name)]);
-      ngModuleStatements.push(callExpr.toStmt());
-    }
     return [
       {
         name: 'ngModuleDef',

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

@@ -484,7 +484,7 @@ describe('ngtsc behavioral tests', () => {
        expect(jsContents).not.toContain('\u0275\u0275setNgModuleScope(TestModule,');
      });
 
-  it('should emit a \u0275registerNgModuleType call when the module has an id', () => {
+  it('should emit the id when the module\'s id is a string', () => {
     env.tsconfig();
     env.write('test.ts', `
         import {NgModule} from '@angular/core';
@@ -496,27 +496,27 @@ describe('ngtsc behavioral tests', () => {
     env.driveMain();
 
     const jsContents = env.getContents('test.js');
-    expect(jsContents).toContain('i0.\u0275registerNgModuleType(\'test\', TestModule);');
+    expect(jsContents).toContain(`i0.\u0275\u0275defineNgModule({ type: TestModule, id: 'test' })`);
   });
 
-  it('should emit a \u0275registerNgModuleType call when the module id is defined as `module.id`',
-     () => {
-       env.tsconfig();
-       env.write('index.d.ts', `
+  it('should emit the id when the module\'s id is defined as `module.id`', () => {
+    env.tsconfig();
+    env.write('index.d.ts', `
          declare const module = {id: string};
        `);
-       env.write('test.ts', `
+    env.write('test.ts', `
          import {NgModule} from '@angular/core';
 
          @NgModule({id: module.id})
          export class TestModule {}
        `);
 
-       env.driveMain();
+    env.driveMain();
 
-       const jsContents = env.getContents('test.js');
-       expect(jsContents).toContain('i0.\u0275registerNgModuleType(module.id, TestModule);');
-     });
+    const jsContents = env.getContents('test.js');
+    expect(jsContents)
+        .toContain('i0.\u0275\u0275defineNgModule({ type: TestModule, id: module.id })');
+  });
 
   it('should filter out directives and pipes from module exports in the injector def', () => {
     env.tsconfig();

packages/compiler/src/compiler_facade_interface.ts

@@ -106,6 +106,7 @@ export interface R3NgModuleMetadataFacade {
   exports: Function[];
   emitInline: boolean;
   schemas: {name: string}[]|null;
+  id: string|null;
 }
 
 export interface R3InjectorMetadataFacade {

packages/compiler/src/jit_compiler_facade.ts

@@ -91,6 +91,7 @@ export class CompilerFacadeImpl implements CompilerFacade {
       emitInline: true,
       containsForwardDecls: false,
       schemas: facade.schemas ? facade.schemas.map(wrapReference) : null,
+      id: facade.id ? new WrappedNodeExpr(facade.id) : null,
     };
     const res = compileNgModule(meta);
     return this.jitExpression(res.expression, angularCoreEnv, sourceMapUrl, []);

packages/compiler/src/render3/r3_identifiers.ts

@@ -239,9 +239,6 @@ export class Identifiers {
     moduleName: CORE,
   };
 
-  static registerNgModuleType:
-      o.ExternalReference = {name: 'ɵregisterNgModuleType', moduleName: CORE};
-
   // sanitization-related functions
   static sanitizeHtml: o.ExternalReference = {name: 'ɵɵsanitizeHtml', moduleName: CORE};
   static sanitizeStyle: o.ExternalReference = {name: 'ɵɵsanitizeStyle', moduleName: CORE};

packages/compiler/src/render3/r3_module_compiler.ts

@@ -67,6 +67,9 @@ export interface R3NgModuleMetadata {
    * The set of schemas that declare elements to be allowed in the NgModule.
    */
   schemas: R3Reference[]|null;
+
+  /** Unique ID or expression representing the unique ID of an NgModule. */
+  id: o.Expression|null;
 }
 
 /**
@@ -81,7 +84,8 @@ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef {
     exports,
     schemas,
     containsForwardDecls,
-    emitInline
+    emitInline,
+    id
   } = meta;
 
   const additionalStatements: o.Statement[] = [];
@@ -93,7 +97,8 @@ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef {
     declarations: o.Expression,
     imports: o.Expression,
     exports: o.Expression,
-    schemas: o.LiteralArrayExpr
+    schemas: o.LiteralArrayExpr,
+    id: o.Expression
   };
 
   // Only generate the keys in the metadata if the arrays have values.
@@ -130,6 +135,10 @@ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef {
     definitionMap.schemas = o.literalArr(schemas.map(ref => ref.value));
   }
 
+  if (id) {
+    definitionMap.id = id;
+  }
+
   const expression = o.importExpr(R3.defineNgModule).callFn([mapToMapExpression(definitionMap)]);
   const type = new o.ExpressionType(o.importExpr(R3.NgModuleDefWithMeta, [
     new o.ExpressionType(moduleType), tupleTypeOf(declarations), tupleTypeOf(imports),

packages/core/src/codegen_private_exports.ts

@@ -7,5 +7,5 @@
  */
 
 export {CodegenComponentFactoryResolver as ɵCodegenComponentFactoryResolver} from './linker/component_factory_resolver';
-export {registerModuleFactory as ɵregisterModuleFactory} from './linker/ng_module_factory_loader';
+export {registerModuleFactory as ɵregisterModuleFactory} from './linker/ng_module_factory_registration';
 export {ArgumentType as ɵArgumentType, BindingFlags as ɵBindingFlags, DepFlags as ɵDepFlags, EMPTY_ARRAY as ɵEMPTY_ARRAY, EMPTY_MAP as ɵEMPTY_MAP, NodeFlags as ɵNodeFlags, QueryBindingType as ɵQueryBindingType, QueryValueType as ɵQueryValueType, ViewDefinition as ɵViewDefinition, ViewFlags as ɵViewFlags, anchorDef as ɵand, createComponentFactory as ɵccf, createNgModuleFactory as ɵcmf, createRendererType2 as ɵcrt, directiveDef as ɵdid, elementDef as ɵeld, getComponentViewDefinitionFactory as ɵgetComponentViewDefinitionFactory, inlineInterpolate as ɵinlineInterpolate, interpolate as ɵinterpolate, moduleDef as ɵmod, moduleProvideDef as ɵmpd, ngContentDef as ɵncd, nodeValue as ɵnov, pipeDef as ɵpid, providerDef as ɵprd, pureArrayDef as ɵpad, pureObjectDef as ɵpod, purePipeDef as ɵppd, queryDef as ɵqud, textDef as ɵted, unwrapValue as ɵunv, viewDef as ɵvid} from './view/index';

packages/core/src/compiler/compiler_facade_interface.ts

@@ -106,6 +106,7 @@ export interface R3NgModuleMetadataFacade {
   exports: Function[];
   emitInline: boolean;
   schemas: {name: string}[]|null;
+  id: string|null;
 }
 
 export interface R3InjectorMetadataFacade {

packages/core/src/core_render3_private_export.ts

@@ -274,10 +274,9 @@ export {
   SWITCH_RENDERER2_FACTORY__POST_R3__ as ɵSWITCH_RENDERER2_FACTORY__POST_R3__,
 } from './render/api';
 
-export {
-  getModuleFactory__POST_R3__ as ɵgetModuleFactory__POST_R3__,
-  registerNgModuleType as ɵregisterNgModuleType,
-} from './linker/ng_module_factory_loader';
+export { getModuleFactory__POST_R3__ as ɵgetModuleFactory__POST_R3__ } from './linker/ng_module_factory_loader';
+
+export { registerNgModuleType as ɵregisterNgModuleType } from './linker/ng_module_factory_registration';
 
 export {
   publishGlobalUtil as ɵpublishGlobalUtil,

packages/core/src/linker/ng_module_factory_loader.ts

@@ -6,11 +6,10 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Type} from '../interface/type';
 import {NgModuleFactory as R3NgModuleFactory, NgModuleType} from '../render3/ng_module_ref';
-import {stringify} from '../util/stringify';
 
 import {NgModuleFactory} from './ng_module_factory';
+import {getRegisteredNgModuleType} from './ng_module_factory_registration';
 
 
 /**
@@ -24,48 +23,14 @@ export abstract class NgModuleFactoryLoader {
   abstract load(path: string): Promise<NgModuleFactory<any>>;
 }
 
-/**
- * Map of module-id to the corresponding NgModule.
- * - In pre Ivy we track NgModuleFactory,
- * - In post Ivy we track the NgModuleType
- */
-const modules = new Map<string, NgModuleFactory<any>|NgModuleType>();
-
-/**
- * Registers a loaded module. Should only be called from generated NgModuleFactory code.
- * @publicApi
- */
-export function registerModuleFactory(id: string, factory: NgModuleFactory<any>) {
-  const existing = modules.get(id) as NgModuleFactory<any>;
-  assertSameOrNotExisting(id, existing && existing.moduleType, factory.moduleType);
-  modules.set(id, factory);
-}
-
-function assertSameOrNotExisting(id: string, type: Type<any>| null, incoming: Type<any>): void {
-  if (type && type !== incoming) {
-    throw new Error(
-        `Duplicate module registered for ${id} - ${stringify(type)} vs ${stringify(type.name)}`);
-  }
-}
-
-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 clearModulesForTest(): void {
-  modules.clear();
-}
-
 export function getModuleFactory__PRE_R3__(id: string): NgModuleFactory<any> {
-  const factory = modules.get(id) as NgModuleFactory<any>| null;
+  const factory = getRegisteredNgModuleType(id) as NgModuleFactory<any>| null;
   if (!factory) throw noModuleError(id);
   return factory;
 }
 
 export function getModuleFactory__POST_R3__(id: string): NgModuleFactory<any> {
-  const type = modules.get(id) as NgModuleType | null;
+  const type = getRegisteredNgModuleType(id) as NgModuleType | null;
   if (!type) throw noModuleError(id);
   return new R3NgModuleFactory(type);
 }

packages/core/src/linker/ng_module_factory_registration.ts

@@ -0,0 +1,52 @@
+/**
+ * @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
+ */
+
+import {Type} from '../interface/type';
+import {NgModuleType} from '../render3/ng_module_ref';
+import {stringify} from '../util/stringify';
+
+import {NgModuleFactory} from './ng_module_factory';
+
+
+/**
+ * Map of module-id to the corresponding NgModule.
+ * - In pre Ivy we track NgModuleFactory,
+ * - In post Ivy we track the NgModuleType
+ */
+const modules = new Map<string, NgModuleFactory<any>|NgModuleType>();
+
+/**
+ * Registers a loaded module. Should only be called from generated NgModuleFactory code.
+ * @publicApi
+ */
+export function registerModuleFactory(id: string, factory: NgModuleFactory<any>) {
+  const existing = modules.get(id) as NgModuleFactory<any>;
+  assertSameOrNotExisting(id, existing && existing.moduleType, factory.moduleType);
+  modules.set(id, factory);
+}
+
+function assertSameOrNotExisting(id: string, type: Type<any>| null, incoming: Type<any>): void {
+  if (type && type !== incoming) {
+    throw new Error(
+        `Duplicate module registered for ${id} - ${stringify(type)} vs ${stringify(type.name)}`);
+  }
+}
+
+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 clearModulesForTest(): void {
+  modules.clear();
+}
+
+export function getRegisteredNgModuleType(id: string) {
+  return modules.get(id);
+}

packages/core/src/metadata/ng_module.ts

@@ -75,6 +75,9 @@ export interface NgModuleDef<T> {
 
   /** The set of schemas that declare elements to be allowed in the NgModule. */
   schemas: SchemaMetadata[]|null;
+
+  /** Unique ID for the module with which it should be registered.  */
+  id: string|null;
 }
 
 /**

packages/core/src/render3/definition.ts

@@ -366,6 +366,9 @@ export function ɵɵdefineNgModule<T>(def: {
 
   /** The set of schemas that declare elements to be allowed in the NgModule. */
   schemas?: SchemaMetadata[] | null;
+
+  /** Unique ID for the module that is used with `getModuleFactory`. */
+  id?: string | null;
 }): never {
   const res: NgModuleDef<T> = {
     type: def.type,
@@ -375,6 +378,7 @@ export function ɵɵdefineNgModule<T>(def: {
     exports: def.exports || EMPTY_ARRAY,
     transitiveCompileScopes: null,
     schemas: def.schemas || null,
+    id: def.id || null,
   };
   return res as never;
 }

packages/core/src/render3/jit/environment.ts

@@ -9,7 +9,6 @@
 import {ɵɵdefineInjectable, ɵɵdefineInjector,} from '../../di/interface/defs';
 import {ɵɵinject} from '../../di/injector_compatibility';
 import * as r3 from '../index';
-import {registerNgModuleType} from '../../linker/ng_module_factory_loader';
 import * as sanitization from '../../sanitization/sanitization';
 
 
@@ -139,6 +138,4 @@ export const angularCoreEnv: {[name: string]: Function} = {
   'ɵɵsanitizeScript': sanitization.ɵɵsanitizeScript,
   'ɵɵsanitizeUrl': sanitization.ɵɵsanitizeUrl,
   'ɵɵsanitizeUrlOrResourceUrl': sanitization.ɵɵsanitizeUrlOrResourceUrl,
-
-  'ɵregisterNgModuleType': registerNgModuleType,
 };

packages/core/src/render3/jit/module.ts

@@ -11,7 +11,6 @@ import {resolveForwardRef} from '../../di/forward_ref';
 import {NG_INJECTOR_DEF} from '../../di/interface/defs';
 import {reflectDependencies} from '../../di/jit/util';
 import {Type} from '../../interface/type';
-import {registerNgModuleType} from '../../linker/ng_module_factory_loader';
 import {Component} from '../../metadata';
 import {ModuleWithProviders, NgModule, NgModuleDef, NgModuleTransitiveScopes} from '../../metadata/ng_module';
 import {flatten} from '../../util/array_utils';
@@ -117,14 +116,12 @@ export function compileNgModuleDefs(moduleType: NgModuleType, ngModule: NgModule
                            .map(expandModuleWithProviders),
               emitInline: true,
               schemas: ngModule.schemas ? flatten(ngModule.schemas) : null,
+              id: ngModule.id || null,
             });
       }
       return ngModuleDef;
     }
   });
-  if (ngModule.id) {
-    registerNgModuleType(ngModule.id, moduleType);
-  }
 
   let ngInjectorDef: any = null;
   Object.defineProperty(moduleType, NG_INJECTOR_DEF, {

packages/core/src/render3/ng_module_ref.ts

@@ -14,9 +14,11 @@ import {R3Injector, createInjector} from '../di/r3_injector';
 import {Type} from '../interface/type';
 import {ComponentFactoryResolver as viewEngine_ComponentFactoryResolver} from '../linker/component_factory_resolver';
 import {InternalNgModuleRef, NgModuleFactory as viewEngine_NgModuleFactory, NgModuleRef as viewEngine_NgModuleRef} from '../linker/ng_module_factory';
+import {registerNgModuleType} from '../linker/ng_module_factory_registration';
 import {NgModuleDef} from '../metadata/ng_module';
 import {assertDefined} from '../util/assert';
 import {stringify} from '../util/stringify';
+
 import {ComponentFactoryResolver} from './component_ref';
 import {getNgModuleDef} from './definition';
 import {maybeUnwrapFn} from './util/misc_utils';
@@ -87,6 +89,11 @@ export class NgModuleFactory<T> extends viewEngine_NgModuleFactory<T> {
   constructor(public moduleType: Type<T>) { super(); }
 
   create(parentInjector: Injector|null): viewEngine_NgModuleRef<T> {
-    return new NgModuleRef(this.moduleType, parentInjector);
+    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;
   }
 }

packages/core/test/linker/ng_module_integration_spec.ts

@@ -17,7 +17,7 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
 import {modifiedInIvy, obsoleteInIvy, onlyInIvy} from '@angular/private/testing';
 
 import {InternalNgModuleRef, NgModuleFactory} from '../../src/linker/ng_module_factory';
-import {clearModulesForTest} from '../../src/linker/ng_module_factory_loader';
+import {clearModulesForTest} from '../../src/linker/ng_module_factory_registration';
 import {stringify} from '../../src/util/stringify';
 
 class Engine {}
@@ -327,6 +327,18 @@ function declareTests(config?: {useJit: boolean}) {
           createModule(SomeOtherModule);
         }).toThrowError(/Duplicate module registered/);
       });
+
+      it('should not throw immediately if two modules have the same id', () => {
+        expect(() => {
+          @NgModule({id: 'some-module'})
+          class ModuleA {
+          }
+
+          @NgModule({id: 'some-module'})
+          class ModuleB {
+          }
+        }).not.toThrow();
+      });
     });
 
     describe('entryComponents', () => {

tools/public_api_guard/core/core.d.ts

@@ -777,6 +777,7 @@ export declare function ɵɵdefineNgModule<T>(def: {
     imports?: Type<any>[] | (() => Type<any>[]);
     exports?: Type<any>[] | (() => Type<any>[]);
     schemas?: SchemaMetadata[] | null;
+    id?: string | null;
 }): never;
 
 export declare function ɵɵdefinePipe<T>(pipeDef: {