Skip to content

Commit f74373f

Browse files
crisbetoalxhub
crisbeto
authored and
alxhub
committed
fix(ivy): align NgModule registration timing with ViewEngine (angular#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 angular#30244
1 parent 2f35dbf commit f74373f

18 files changed

+115
-73
lines changed

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
162162
exports,
163163
imports,
164164
containsForwardDecls,
165+
id,
165166
emitInline: false,
166167
// TODO: to be implemented as a part of FW-1004.
167168
schemas: [],
@@ -257,11 +258,6 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
257258
ngModuleStatements.push(callExpr.toStmt());
258259
}
259260
}
260-
if (analysis.id !== null) {
261-
const registerNgModuleType = new ExternalExpr(R3Identifiers.registerNgModuleType);
262-
const callExpr = registerNgModuleType.callFn([analysis.id, new WrappedNodeExpr(node.name)]);
263-
ngModuleStatements.push(callExpr.toStmt());
264-
}
265261
return [
266262
{
267263
name: 'ngModuleDef',

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

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ describe('ngtsc behavioral tests', () => {
484484
expect(jsContents).not.toContain('\u0275\u0275setNgModuleScope(TestModule,');
485485
});
486486

487-
it('should emit a \u0275registerNgModuleType call when the module has an id', () => {
487+
it('should emit the id when the module\'s id is a string', () => {
488488
env.tsconfig();
489489
env.write('test.ts', `
490490
import {NgModule} from '@angular/core';
@@ -496,27 +496,27 @@ describe('ngtsc behavioral tests', () => {
496496
env.driveMain();
497497

498498
const jsContents = env.getContents('test.js');
499-
expect(jsContents).toContain('i0.\u0275registerNgModuleType(\'test\', TestModule);');
499+
expect(jsContents).toContain(`i0.\u0275\u0275defineNgModule({ type: TestModule, id: 'test' })`);
500500
});
501501

502-
it('should emit a \u0275registerNgModuleType call when the module id is defined as `module.id`',
503-
() => {
504-
env.tsconfig();
505-
env.write('index.d.ts', `
502+
it('should emit the id when the module\'s id is defined as `module.id`', () => {
503+
env.tsconfig();
504+
env.write('index.d.ts', `
506505
declare const module = {id: string};
507506
`);
508-
env.write('test.ts', `
507+
env.write('test.ts', `
509508
import {NgModule} from '@angular/core';
510509
511510
@NgModule({id: module.id})
512511
export class TestModule {}
513512
`);
514513

515-
env.driveMain();
514+
env.driveMain();
516515

517-
const jsContents = env.getContents('test.js');
518-
expect(jsContents).toContain('i0.\u0275registerNgModuleType(module.id, TestModule);');
519-
});
516+
const jsContents = env.getContents('test.js');
517+
expect(jsContents)
518+
.toContain('i0.\u0275\u0275defineNgModule({ type: TestModule, id: module.id })');
519+
});
520520

521521
it('should filter out directives and pipes from module exports in the injector def', () => {
522522
env.tsconfig();

‎packages/compiler/src/compiler_facade_interface.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ export interface R3NgModuleMetadataFacade {
106106
exports: Function[];
107107
emitInline: boolean;
108108
schemas: {name: string}[]|null;
109+
id: string|null;
109110
}
110111

111112
export interface R3InjectorMetadataFacade {

‎packages/compiler/src/jit_compiler_facade.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ export class CompilerFacadeImpl implements CompilerFacade {
9191
emitInline: true,
9292
containsForwardDecls: false,
9393
schemas: facade.schemas ? facade.schemas.map(wrapReference) : null,
94+
id: facade.id ? new WrappedNodeExpr(facade.id) : null,
9495
};
9596
const res = compileNgModule(meta);
9697
return this.jitExpression(res.expression, angularCoreEnv, sourceMapUrl, []);

‎packages/compiler/src/render3/r3_identifiers.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -239,9 +239,6 @@ export class Identifiers {
239239
moduleName: CORE,
240240
};
241241

242-
static registerNgModuleType:
243-
o.ExternalReference = {name: 'ɵregisterNgModuleType', moduleName: CORE};
244-
245242
// sanitization-related functions
246243
static sanitizeHtml: o.ExternalReference = {name: 'ɵɵsanitizeHtml', moduleName: CORE};
247244
static sanitizeStyle: o.ExternalReference = {name: 'ɵɵsanitizeStyle', moduleName: CORE};

‎packages/compiler/src/render3/r3_module_compiler.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ export interface R3NgModuleMetadata {
6767
* The set of schemas that declare elements to be allowed in the NgModule.
6868
*/
6969
schemas: R3Reference[]|null;
70+
71+
/** Unique ID or expression representing the unique ID of an NgModule. */
72+
id: o.Expression|null;
7073
}
7174

7275
/**
@@ -81,7 +84,8 @@ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef {
8184
exports,
8285
schemas,
8386
containsForwardDecls,
84-
emitInline
87+
emitInline,
88+
id
8589
} = meta;
8690

8791
const additionalStatements: o.Statement[] = [];
@@ -93,7 +97,8 @@ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef {
9397
declarations: o.Expression,
9498
imports: o.Expression,
9599
exports: o.Expression,
96-
schemas: o.LiteralArrayExpr
100+
schemas: o.LiteralArrayExpr,
101+
id: o.Expression
97102
};
98103

99104
// Only generate the keys in the metadata if the arrays have values.
@@ -130,6 +135,10 @@ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef {
130135
definitionMap.schemas = o.literalArr(schemas.map(ref => ref.value));
131136
}
132137

138+
if (id) {
139+
definitionMap.id = id;
140+
}
141+
133142
const expression = o.importExpr(R3.defineNgModule).callFn([mapToMapExpression(definitionMap)]);
134143
const type = new o.ExpressionType(o.importExpr(R3.NgModuleDefWithMeta, [
135144
new o.ExpressionType(moduleType), tupleTypeOf(declarations), tupleTypeOf(imports),

‎packages/core/src/codegen_private_exports.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,5 @@
77
*/
88

99
export {CodegenComponentFactoryResolver as ɵCodegenComponentFactoryResolver} from './linker/component_factory_resolver';
10-
export {registerModuleFactory as ɵregisterModuleFactory} from './linker/ng_module_factory_loader';
10+
export {registerModuleFactory as ɵregisterModuleFactory} from './linker/ng_module_factory_registration';
1111
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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ export interface R3NgModuleMetadataFacade {
106106
exports: Function[];
107107
emitInline: boolean;
108108
schemas: {name: string}[]|null;
109+
id: string|null;
109110
}
110111

111112
export interface R3InjectorMetadataFacade {

‎packages/core/src/core_render3_private_export.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -274,10 +274,9 @@ export {
274274
SWITCH_RENDERER2_FACTORY__POST_R3__ as ɵSWITCH_RENDERER2_FACTORY__POST_R3__,
275275
} from './render/api';
276276

277-
export {
278-
getModuleFactory__POST_R3__ as ɵgetModuleFactory__POST_R3__,
279-
registerNgModuleType as ɵregisterNgModuleType,
280-
} from './linker/ng_module_factory_loader';
277+
export { getModuleFactory__POST_R3__ as ɵgetModuleFactory__POST_R3__ } from './linker/ng_module_factory_loader';
278+
279+
export { registerNgModuleType as ɵregisterNgModuleType } from './linker/ng_module_factory_registration';
281280

282281
export {
283282
publishGlobalUtil as ɵpublishGlobalUtil,

‎packages/core/src/linker/ng_module_factory_loader.ts

Lines changed: 3 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,10 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {Type} from '../interface/type';
109
import {NgModuleFactory as R3NgModuleFactory, NgModuleType} from '../render3/ng_module_ref';
11-
import {stringify} from '../util/stringify';
1210

1311
import {NgModuleFactory} from './ng_module_factory';
12+
import {getRegisteredNgModuleType} from './ng_module_factory_registration';
1413

1514

1615
/**
@@ -24,48 +23,14 @@ export abstract class NgModuleFactoryLoader {
2423
abstract load(path: string): Promise<NgModuleFactory<any>>;
2524
}
2625

27-
/**
28-
* Map of module-id to the corresponding NgModule.
29-
* - In pre Ivy we track NgModuleFactory,
30-
* - In post Ivy we track the NgModuleType
31-
*/
32-
const modules = new Map<string, NgModuleFactory<any>|NgModuleType>();
33-
34-
/**
35-
* Registers a loaded module. Should only be called from generated NgModuleFactory code.
36-
* @publicApi
37-
*/
38-
export function registerModuleFactory(id: string, factory: NgModuleFactory<any>) {
39-
const existing = modules.get(id) as NgModuleFactory<any>;
40-
assertSameOrNotExisting(id, existing && existing.moduleType, factory.moduleType);
41-
modules.set(id, factory);
42-
}
43-
44-
function assertSameOrNotExisting(id: string, type: Type<any>| null, incoming: Type<any>): void {
45-
if (type && type !== incoming) {
46-
throw new Error(
47-
`Duplicate module registered for ${id} - ${stringify(type)} vs ${stringify(type.name)}`);
48-
}
49-
}
50-
51-
export function registerNgModuleType(id: string, ngModuleType: NgModuleType) {
52-
const existing = modules.get(id) as NgModuleType | null;
53-
assertSameOrNotExisting(id, existing, ngModuleType);
54-
modules.set(id, ngModuleType);
55-
}
56-
57-
export function clearModulesForTest(): void {
58-
modules.clear();
59-
}
60-
6126
export function getModuleFactory__PRE_R3__(id: string): NgModuleFactory<any> {
62-
const factory = modules.get(id) as NgModuleFactory<any>| null;
27+
const factory = getRegisteredNgModuleType(id) as NgModuleFactory<any>| null;
6328
if (!factory) throw noModuleError(id);
6429
return factory;
6530
}
6631

6732
export function getModuleFactory__POST_R3__(id: string): NgModuleFactory<any> {
68-
const type = modules.get(id) as NgModuleType | null;
33+
const type = getRegisteredNgModuleType(id) as NgModuleType | null;
6934
if (!type) throw noModuleError(id);
7035
return new R3NgModuleFactory(type);
7136
}

‎packages/core/src/linker/ng_module_factory_registration.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {Type} from '../interface/type';
10+
import {NgModuleType} from '../render3/ng_module_ref';
11+
import {stringify} from '../util/stringify';
12+
13+
import {NgModuleFactory} from './ng_module_factory';
14+
15+
16+
/**
17+
* Map of module-id to the corresponding NgModule.
18+
* - In pre Ivy we track NgModuleFactory,
19+
* - In post Ivy we track the NgModuleType
20+
*/
21+
const modules = new Map<string, NgModuleFactory<any>|NgModuleType>();
22+
23+
/**
24+
* Registers a loaded module. Should only be called from generated NgModuleFactory code.
25+
* @publicApi
26+
*/
27+
export function registerModuleFactory(id: string, factory: NgModuleFactory<any>) {
28+
const existing = modules.get(id) as NgModuleFactory<any>;
29+
assertSameOrNotExisting(id, existing && existing.moduleType, factory.moduleType);
30+
modules.set(id, factory);
31+
}
32+
33+
function assertSameOrNotExisting(id: string, type: Type<any>| null, incoming: Type<any>): void {
34+
if (type && type !== incoming) {
35+
throw new Error(
36+
`Duplicate module registered for ${id} - ${stringify(type)} vs ${stringify(type.name)}`);
37+
}
38+
}
39+
40+
export function registerNgModuleType(id: string, ngModuleType: NgModuleType) {
41+
const existing = modules.get(id) as NgModuleType | null;
42+
assertSameOrNotExisting(id, existing, ngModuleType);
43+
modules.set(id, ngModuleType);
44+
}
45+
46+
export function clearModulesForTest(): void {
47+
modules.clear();
48+
}
49+
50+
export function getRegisteredNgModuleType(id: string) {
51+
return modules.get(id);
52+
}

‎packages/core/src/metadata/ng_module.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ export interface NgModuleDef<T> {
7575

7676
/** The set of schemas that declare elements to be allowed in the NgModule. */
7777
schemas: SchemaMetadata[]|null;
78+
79+
/** Unique ID for the module with which it should be registered. */
80+
id: string|null;
7881
}
7982

8083
/**

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,9 @@ export function ɵɵdefineNgModule<T>(def: {
366366

367367
/** The set of schemas that declare elements to be allowed in the NgModule. */
368368
schemas?: SchemaMetadata[] | null;
369+
370+
/** Unique ID for the module that is used with `getModuleFactory`. */
371+
id?: string | null;
369372
}): never {
370373
const res: NgModuleDef<T> = {
371374
type: def.type,
@@ -375,6 +378,7 @@ export function ɵɵdefineNgModule<T>(def: {
375378
exports: def.exports || EMPTY_ARRAY,
376379
transitiveCompileScopes: null,
377380
schemas: def.schemas || null,
381+
id: def.id || null,
378382
};
379383
return res as never;
380384
}

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import {ɵɵdefineInjectable, ɵɵdefineInjector,} from '../../di/interface/defs';
1010
import {ɵɵinject} from '../../di/injector_compatibility';
1111
import * as r3 from '../index';
12-
import {registerNgModuleType} from '../../linker/ng_module_factory_loader';
1312
import * as sanitization from '../../sanitization/sanitization';
1413

1514

@@ -139,6 +138,4 @@ export const angularCoreEnv: {[name: string]: Function} = {
139138
'ɵɵsanitizeScript': sanitization.ɵɵsanitizeScript,
140139
'ɵɵsanitizeUrl': sanitization.ɵɵsanitizeUrl,
141140
'ɵɵsanitizeUrlOrResourceUrl': sanitization.ɵɵsanitizeUrlOrResourceUrl,
142-
143-
'ɵregisterNgModuleType': registerNgModuleType,
144141
};

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import {resolveForwardRef} from '../../di/forward_ref';
1111
import {NG_INJECTOR_DEF} from '../../di/interface/defs';
1212
import {reflectDependencies} from '../../di/jit/util';
1313
import {Type} from '../../interface/type';
14-
import {registerNgModuleType} from '../../linker/ng_module_factory_loader';
1514
import {Component} from '../../metadata';
1615
import {ModuleWithProviders, NgModule, NgModuleDef, NgModuleTransitiveScopes} from '../../metadata/ng_module';
1716
import {flatten} from '../../util/array_utils';
@@ -117,14 +116,12 @@ export function compileNgModuleDefs(moduleType: NgModuleType, ngModule: NgModule
117116
.map(expandModuleWithProviders),
118117
emitInline: true,
119118
schemas: ngModule.schemas ? flatten(ngModule.schemas) : null,
119+
id: ngModule.id || null,
120120
});
121121
}
122122
return ngModuleDef;
123123
}
124124
});
125-
if (ngModule.id) {
126-
registerNgModuleType(ngModule.id, moduleType);
127-
}
128125

129126
let ngInjectorDef: any = null;
130127
Object.defineProperty(moduleType, NG_INJECTOR_DEF, {

‎packages/core/src/render3/ng_module_ref.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@ import {R3Injector, createInjector} from '../di/r3_injector';
1414
import {Type} from '../interface/type';
1515
import {ComponentFactoryResolver as viewEngine_ComponentFactoryResolver} from '../linker/component_factory_resolver';
1616
import {InternalNgModuleRef, NgModuleFactory as viewEngine_NgModuleFactory, NgModuleRef as viewEngine_NgModuleRef} from '../linker/ng_module_factory';
17+
import {registerNgModuleType} from '../linker/ng_module_factory_registration';
1718
import {NgModuleDef} from '../metadata/ng_module';
1819
import {assertDefined} from '../util/assert';
1920
import {stringify} from '../util/stringify';
21+
2022
import {ComponentFactoryResolver} from './component_ref';
2123
import {getNgModuleDef} from './definition';
2224
import {maybeUnwrapFn} from './util/misc_utils';
@@ -87,6 +89,11 @@ export class NgModuleFactory<T> extends viewEngine_NgModuleFactory<T> {
8789
constructor(public moduleType: Type<T>) { super(); }
8890

8991
create(parentInjector: Injector|null): viewEngine_NgModuleRef<T> {
90-
return new NgModuleRef(this.moduleType, parentInjector);
92+
const moduleType = this.moduleType;
93+
const moduleRef = new NgModuleRef(moduleType, parentInjector);
94+
const ngModuleDef = getNgModuleDef(moduleType);
95+
ngModuleDef && ngModuleDef.id &&
96+
registerNgModuleType(ngModuleDef.id, moduleType as NgModuleType);
97+
return moduleRef;
9198
}
9299
}

‎packages/core/test/linker/ng_module_integration_spec.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
1717
import {modifiedInIvy, obsoleteInIvy, onlyInIvy} from '@angular/private/testing';
1818

1919
import {InternalNgModuleRef, NgModuleFactory} from '../../src/linker/ng_module_factory';
20-
import {clearModulesForTest} from '../../src/linker/ng_module_factory_loader';
20+
import {clearModulesForTest} from '../../src/linker/ng_module_factory_registration';
2121
import {stringify} from '../../src/util/stringify';
2222

2323
class Engine {}
@@ -327,6 +327,18 @@ function declareTests(config?: {useJit: boolean}) {
327327
createModule(SomeOtherModule);
328328
}).toThrowError(/Duplicate module registered/);
329329
});
330+
331+
it('should not throw immediately if two modules have the same id', () => {
332+
expect(() => {
333+
@NgModule({id: 'some-module'})
334+
class ModuleA {
335+
}
336+
337+
@NgModule({id: 'some-module'})
338+
class ModuleB {
339+
}
340+
}).not.toThrow();
341+
});
330342
});
331343

332344
describe('entryComponents', () => {

0 commit comments

Comments
 (0)