From f74373f2dd829249e77537a90883eda833a8ce85 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 7 May 2019 22:57:55 -0400 Subject: [PATCH] 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 --- .../src/ngtsc/annotations/src/ng_module.ts | 6 +-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 22 ++++---- .../compiler/src/compiler_facade_interface.ts | 1 + packages/compiler/src/jit_compiler_facade.ts | 1 + .../compiler/src/render3/r3_identifiers.ts | 3 -- .../src/render3/r3_module_compiler.ts | 13 ++++- packages/core/src/codegen_private_exports.ts | 2 +- .../src/compiler/compiler_facade_interface.ts | 1 + .../core/src/core_render3_private_export.ts | 7 ++- .../src/linker/ng_module_factory_loader.ts | 41 ++------------- .../linker/ng_module_factory_registration.ts | 52 +++++++++++++++++++ packages/core/src/metadata/ng_module.ts | 3 ++ packages/core/src/render3/definition.ts | 4 ++ packages/core/src/render3/jit/environment.ts | 3 -- packages/core/src/render3/jit/module.ts | 5 +- packages/core/src/render3/ng_module_ref.ts | 9 +++- .../test/linker/ng_module_integration_spec.ts | 14 ++++- tools/public_api_guard/core/core.d.ts | 1 + 18 files changed, 115 insertions(+), 73 deletions(-) create mode 100644 packages/core/src/linker/ng_module_factory_registration.ts diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index f65b93db86..52040dfd1e 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -162,6 +162,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler { 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(); diff --git a/packages/compiler/src/compiler_facade_interface.ts b/packages/compiler/src/compiler_facade_interface.ts index 7214522ddd..8fb8a44d07 100644 --- a/packages/compiler/src/compiler_facade_interface.ts +++ b/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 { diff --git a/packages/compiler/src/jit_compiler_facade.ts b/packages/compiler/src/jit_compiler_facade.ts index 4e135ad90a..c14a4680bf 100644 --- a/packages/compiler/src/jit_compiler_facade.ts +++ b/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, []); diff --git a/packages/compiler/src/render3/r3_identifiers.ts b/packages/compiler/src/render3/r3_identifiers.ts index 7e872d9a3e..af400aa98c 100644 --- a/packages/compiler/src/render3/r3_identifiers.ts +++ b/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}; diff --git a/packages/compiler/src/render3/r3_module_compiler.ts b/packages/compiler/src/render3/r3_module_compiler.ts index 213e762c5f..df83a468d6 100644 --- a/packages/compiler/src/render3/r3_module_compiler.ts +++ b/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), diff --git a/packages/core/src/codegen_private_exports.ts b/packages/core/src/codegen_private_exports.ts index fffbd4f4fa..177d4560ab 100644 --- a/packages/core/src/codegen_private_exports.ts +++ b/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'; diff --git a/packages/core/src/compiler/compiler_facade_interface.ts b/packages/core/src/compiler/compiler_facade_interface.ts index 7214522ddd..8fb8a44d07 100644 --- a/packages/core/src/compiler/compiler_facade_interface.ts +++ b/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 { diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts index c7374775ac..f2b48983e4 100644 --- a/packages/core/src/core_render3_private_export.ts +++ b/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, diff --git a/packages/core/src/linker/ng_module_factory_loader.ts b/packages/core/src/linker/ng_module_factory_loader.ts index f5a8b9aec6..2f6f710c6e 100644 --- a/packages/core/src/linker/ng_module_factory_loader.ts +++ b/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>; } -/** - * 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|NgModuleType>(); - -/** - * Registers a loaded module. Should only be called from generated NgModuleFactory code. - * @publicApi - */ -export function registerModuleFactory(id: string, factory: NgModuleFactory) { - const existing = modules.get(id) as NgModuleFactory; - assertSameOrNotExisting(id, existing && existing.moduleType, factory.moduleType); - modules.set(id, factory); -} - -function assertSameOrNotExisting(id: string, type: Type| null, incoming: Type): 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 { - const factory = modules.get(id) as NgModuleFactory| null; + const factory = getRegisteredNgModuleType(id) as NgModuleFactory| null; if (!factory) throw noModuleError(id); return factory; } export function getModuleFactory__POST_R3__(id: string): NgModuleFactory { - const type = modules.get(id) as NgModuleType | null; + const type = getRegisteredNgModuleType(id) as NgModuleType | null; if (!type) throw noModuleError(id); return new R3NgModuleFactory(type); } diff --git a/packages/core/src/linker/ng_module_factory_registration.ts b/packages/core/src/linker/ng_module_factory_registration.ts new file mode 100644 index 0000000000..e63314323f --- /dev/null +++ b/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|NgModuleType>(); + +/** + * Registers a loaded module. Should only be called from generated NgModuleFactory code. + * @publicApi + */ +export function registerModuleFactory(id: string, factory: NgModuleFactory) { + const existing = modules.get(id) as NgModuleFactory; + assertSameOrNotExisting(id, existing && existing.moduleType, factory.moduleType); + modules.set(id, factory); +} + +function assertSameOrNotExisting(id: string, type: Type| null, incoming: Type): 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); +} diff --git a/packages/core/src/metadata/ng_module.ts b/packages/core/src/metadata/ng_module.ts index 36201f1726..0b7638f0ad 100644 --- a/packages/core/src/metadata/ng_module.ts +++ b/packages/core/src/metadata/ng_module.ts @@ -75,6 +75,9 @@ export interface NgModuleDef { /** 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; } /** diff --git a/packages/core/src/render3/definition.ts b/packages/core/src/render3/definition.ts index ecefde30c9..a5eb841056 100644 --- a/packages/core/src/render3/definition.ts +++ b/packages/core/src/render3/definition.ts @@ -366,6 +366,9 @@ export function ɵɵdefineNgModule(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 = { type: def.type, @@ -375,6 +378,7 @@ export function ɵɵdefineNgModule(def: { exports: def.exports || EMPTY_ARRAY, transitiveCompileScopes: null, schemas: def.schemas || null, + id: def.id || null, }; return res as never; } diff --git a/packages/core/src/render3/jit/environment.ts b/packages/core/src/render3/jit/environment.ts index 3dc719945e..781ed99f83 100644 --- a/packages/core/src/render3/jit/environment.ts +++ b/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, }; diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index 3777b0cf1c..324eef50e8 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/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, { diff --git a/packages/core/src/render3/ng_module_ref.ts b/packages/core/src/render3/ng_module_ref.ts index 889548a86b..07518d77f2 100644 --- a/packages/core/src/render3/ng_module_ref.ts +++ b/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 extends viewEngine_NgModuleFactory { constructor(public moduleType: Type) { super(); } create(parentInjector: Injector|null): viewEngine_NgModuleRef { - 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; } } diff --git a/packages/core/test/linker/ng_module_integration_spec.ts b/packages/core/test/linker/ng_module_integration_spec.ts index 4500bd26df..50061efc78 100644 --- a/packages/core/test/linker/ng_module_integration_spec.ts +++ b/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', () => { diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts index ba7e436cf3..1a3c687730 100644 --- a/tools/public_api_guard/core/core.d.ts +++ b/tools/public_api_guard/core/core.d.ts @@ -777,6 +777,7 @@ export declare function ɵɵdefineNgModule(def: { imports?: Type[] | (() => Type[]); exports?: Type[] | (() => Type[]); schemas?: SchemaMetadata[] | null; + id?: string | null; }): never; export declare function ɵɵdefinePipe(pipeDef: {