From 73da2792c95b0e99b23d8e86f381bc1f2e5d818a Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 8 Mar 2019 17:57:34 -0800 Subject: [PATCH] fix(ivy): properly compile NgModules with forward referenced types (#29198) Previously, ngtsc would resolve forward references while evaluating the bootstrap, declaration, imports, and exports fields of NgModule types. However, when generating the resulting ngModuleDef, the forward nature of these references was not taken into consideration, and so the generated JS code would incorrectly reference types not yet declared. This commit fixes this issue by introducing function closures in the NgModuleDef type, similarly to how NgComponentDef uses them for forward declarations of its directives and pipes arrays. ngtsc will then generate closures when required, and the runtime will unwrap them if present. PR Close #29198 --- .../src/ngtsc/annotations/src/component.ts | 16 +--- .../src/ngtsc/annotations/src/ng_module.ts | 48 +++++++---- .../src/ngtsc/annotations/src/util.ts | 14 ++++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 80 +++++++++++++++++++ packages/compiler/src/jit_compiler_facade.ts | 1 + .../src/render3/r3_module_compiler.ts | 37 ++++++--- packages/core/src/linker/compiler.ts | 13 +-- packages/core/src/metadata/ng_module.ts | 8 +- packages/core/src/render3/jit/module.ts | 28 ++++--- packages/core/src/render3/ng_module_ref.ts | 3 +- packages/core/src/render3/util/misc_utils.ts | 11 +++ packages/core/test/render3/ivy/jit_spec.ts | 3 + packages/core/testing/src/r3_test_bed.ts | 13 ++- 13 files changed, 210 insertions(+), 65 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 9197e756c5..cbbf3ddf53 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -23,7 +23,7 @@ import {tsSourceMapBug29300Fixed} from '../../util/src/ts_source_map_bug_29300'; import {ResourceLoader} from './api'; import {extractDirectiveMetadata, extractQueriesFromDecorator, parseFieldArrayValue, queriesFromFields} from './directive'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, isAngularCoreReference, unwrapExpression} from './util'; +import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, unwrapExpression} from './util'; const EMPTY_MAP = new Map(); const EMPTY_ARRAY: any[] = []; @@ -616,20 +616,6 @@ function getTemplateRange(templateExpr: ts.Expression) { }; } -function isExpressionForwardReference( - expr: Expression, context: ts.Node, contextSource: ts.SourceFile): boolean { - if (isWrappedTsNodeExpr(expr)) { - const node = ts.getOriginalNode(expr.node); - return node.getSourceFile() === contextSource && context.pos < node.pos; - } else { - return false; - } -} - -function isWrappedTsNodeExpr(expr: Expression): expr is WrappedNodeExpr { - return expr instanceof WrappedNodeExpr; -} - function sourceMapUrl(resourceUrl: string): string { if (!tsSourceMapBug29300Fixed()) { // By removing the template URL we are telling the translator not to try to 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 b609707f81..664256c06d 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -20,7 +20,7 @@ import {getSourceFile} from '../../util/src/typescript'; import {generateSetClassMetadataCall} from './metadata'; import {ReferencesRegistry} from './references_registry'; -import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, toR3Reference, unwrapExpression} from './util'; +import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression} from './util'; export interface NgModuleAnalysis { ngModuleDef: R3NgModuleMetadata; @@ -90,38 +90,39 @@ export class NgModuleDecoratorHandler implements DecoratorHandler[] = []; + let declarationRefs: Reference[] = []; if (ngModule.has('declarations')) { const expr = ngModule.get('declarations') !; const declarationMeta = this.evaluator.evaluate(expr, forwardRefResolver); - declarations = this.resolveTypeList(expr, declarationMeta, name, 'declarations'); + declarationRefs = this.resolveTypeList(expr, declarationMeta, name, 'declarations'); } - let imports: Reference[] = []; + let importRefs: Reference[] = []; let rawImports: ts.Expression|null = null; if (ngModule.has('imports')) { rawImports = ngModule.get('imports') !; const importsMeta = this.evaluator.evaluate(rawImports, moduleResolvers); - imports = this.resolveTypeList(rawImports, importsMeta, name, 'imports'); + importRefs = this.resolveTypeList(rawImports, importsMeta, name, 'imports'); } - let exports: Reference[] = []; + let exportRefs: Reference[] = []; let rawExports: ts.Expression|null = null; if (ngModule.has('exports')) { rawExports = ngModule.get('exports') !; const exportsMeta = this.evaluator.evaluate(rawExports, moduleResolvers); - exports = this.resolveTypeList(rawExports, exportsMeta, name, 'exports'); - this.referencesRegistry.add(node, ...exports); + exportRefs = this.resolveTypeList(rawExports, exportsMeta, name, 'exports'); + this.referencesRegistry.add(node, ...exportRefs); } - let bootstrap: Reference[] = []; + let bootstrapRefs: Reference[] = []; if (ngModule.has('bootstrap')) { const expr = ngModule.get('bootstrap') !; const bootstrapMeta = this.evaluator.evaluate(expr, forwardRefResolver); - bootstrap = this.resolveTypeList(expr, bootstrapMeta, name, 'bootstrap'); + bootstrapRefs = this.resolveTypeList(expr, bootstrapMeta, name, 'bootstrap'); } // Register this module's information with the LocalModuleScopeRegistry. This ensures that // during the compile() phase, the module's metadata is available for selector scope // computation. - this.scopeRegistry.registerNgModule(node, {declarations, imports, exports}); + this.scopeRegistry.registerNgModule( + node, {declarations: declarationRefs, imports: importRefs, exports: exportRefs}); const valueContext = node.getSourceFile(); @@ -131,13 +132,26 @@ export class NgModuleDecoratorHandler implements DecoratorHandler this._toR3Reference(bootstrap, valueContext, typeContext)); + const declarations = + declarationRefs.map(decl => this._toR3Reference(decl, valueContext, typeContext)); + const imports = importRefs.map(imp => this._toR3Reference(imp, valueContext, typeContext)); + const exports = exportRefs.map(exp => this._toR3Reference(exp, valueContext, typeContext)); + + const isForwardReference = (ref: R3Reference) => + isExpressionForwardReference(ref.value, node.name !, valueContext); + const containsForwardDecls = bootstrap.some(isForwardReference) || + declarations.some(isForwardReference) || imports.some(isForwardReference) || + exports.some(isForwardReference); + const ngModuleDef: R3NgModuleMetadata = { type: new WrappedNodeExpr(node.name !), - bootstrap: - bootstrap.map(bootstrap => this._toR3Reference(bootstrap, valueContext, typeContext)), - declarations: declarations.map(decl => this._toR3Reference(decl, valueContext, typeContext)), - exports: exports.map(exp => this._toR3Reference(exp, valueContext, typeContext)), - imports: imports.map(imp => this._toR3Reference(imp, valueContext, typeContext)), + bootstrap, + declarations, + exports, + imports, + containsForwardDecls, emitInline: false, // TODO: to be implemented as a part of FW-1004. schemas: [], @@ -173,7 +187,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler { + return expr instanceof WrappedNodeExpr; +} diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index cdacd28426..f544645053 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -643,6 +643,86 @@ describe('ngtsc behavioral tests', () => { expect(dtsContents).toContain('as i1 from "foo";'); }); + it('should compile NgModules with references to forward declared bootstrap components', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Component, forwardRef, NgModule} from '@angular/core'; + + @NgModule({ + bootstrap: [forwardRef(() => Foo)], + }) + export class FooModule {} + + @Component({selector: 'foo', template: 'foo'}) + export class Foo {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('bootstrap: function () { return [Foo]; }'); + }); + + it('should compile NgModules with references to forward declared directives', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Directive, forwardRef, NgModule} from '@angular/core'; + + @NgModule({ + declarations: [forwardRef(() => Foo)], + }) + export class FooModule {} + + @Directive({selector: 'foo'}) + export class Foo {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('declarations: function () { return [Foo]; }'); + }); + + it('should compile NgModules with references to forward declared imports', () => { + env.tsconfig(); + env.write('test.ts', ` + import {forwardRef, NgModule} from '@angular/core'; + + @NgModule({ + imports: [forwardRef(() => BarModule)], + }) + export class FooModule {} + + @NgModule({}) + export class BarModule {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('imports: function () { return [BarModule]; }'); + }); + + it('should compile NgModules with references to forward declared exports', () => { + env.tsconfig(); + env.write('test.ts', ` + import {forwardRef, NgModule} from '@angular/core'; + + @NgModule({ + exports: [forwardRef(() => BarModule)], + }) + export class FooModule {} + + @NgModule({}) + export class BarModule {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('exports: function () { return [BarModule]; }'); + }); + it('should compile Pipes without errors', () => { env.tsconfig(); env.write('test.ts', ` diff --git a/packages/compiler/src/jit_compiler_facade.ts b/packages/compiler/src/jit_compiler_facade.ts index c805b5d2c4..c7fbfe50db 100644 --- a/packages/compiler/src/jit_compiler_facade.ts +++ b/packages/compiler/src/jit_compiler_facade.ts @@ -88,6 +88,7 @@ export class CompilerFacadeImpl implements CompilerFacade { imports: facade.imports.map(wrapReference), exports: facade.exports.map(wrapReference), emitInline: true, + containsForwardDecls: false, schemas: facade.schemas ? facade.schemas.map(wrapReference) : null, }; const res = compileNgModule(meta); diff --git a/packages/compiler/src/render3/r3_module_compiler.ts b/packages/compiler/src/render3/r3_module_compiler.ts index 4a4a3f51d9..14a5bf0ba5 100644 --- a/packages/compiler/src/render3/r3_module_compiler.ts +++ b/packages/compiler/src/render3/r3_module_compiler.ts @@ -58,6 +58,11 @@ export interface R3NgModuleMetadata { */ emitInline: boolean; + /** + * Whether to generate closure wrappers for bootstrap, declarations, imports, and exports. + */ + containsForwardDecls: boolean; + /** * The set of schemas that declare elements to be allowed in the NgModule. */ @@ -68,33 +73,42 @@ export interface R3NgModuleMetadata { * Construct an `R3NgModuleDef` for the given `R3NgModuleMetadata`. */ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef { - const {type: moduleType, bootstrap, declarations, imports, exports, schemas} = meta; + const { + type: moduleType, + bootstrap, + declarations, + imports, + exports, + schemas, + containsForwardDecls + } = meta; + const definitionMap = { type: moduleType } as{ type: o.Expression, - bootstrap: o.LiteralArrayExpr, - declarations: o.LiteralArrayExpr, - imports: o.LiteralArrayExpr, - exports: o.LiteralArrayExpr, + bootstrap: o.Expression, + declarations: o.Expression, + imports: o.Expression, + exports: o.Expression, schemas: o.LiteralArrayExpr }; // Only generate the keys in the metadata if the arrays have values. if (bootstrap.length) { - definitionMap.bootstrap = o.literalArr(bootstrap.map(ref => ref.value)); + definitionMap.bootstrap = refsToArray(bootstrap, containsForwardDecls); } if (declarations.length) { - definitionMap.declarations = o.literalArr(declarations.map(ref => ref.value)); + definitionMap.declarations = refsToArray(declarations, containsForwardDecls); } if (imports.length) { - definitionMap.imports = o.literalArr(imports.map(ref => ref.value)); + definitionMap.imports = refsToArray(imports, containsForwardDecls); } if (exports.length) { - definitionMap.exports = o.literalArr(exports.map(ref => ref.value)); + definitionMap.exports = refsToArray(exports, containsForwardDecls); } if (schemas && schemas.length) { @@ -182,3 +196,8 @@ function tupleTypeOf(exp: R3Reference[]): o.Type { const types = exp.map(ref => o.typeofExpr(ref.type)); return exp.length > 0 ? o.expressionType(o.literalArr(types)) : o.NONE_TYPE; } + +function refsToArray(refs: R3Reference[], shouldForwardDeclare: boolean): o.Expression { + const values = o.literalArr(refs.map(ref => ref.value)); + return shouldForwardDeclare ? o.fn([], [new o.ReturnStatement(values)]) : values; +} \ No newline at end of file diff --git a/packages/core/src/linker/compiler.ts b/packages/core/src/linker/compiler.ts index 25d572667c..c6ad53f774 100644 --- a/packages/core/src/linker/compiler.ts +++ b/packages/core/src/linker/compiler.ts @@ -15,6 +15,7 @@ import {ViewEncapsulation} from '../metadata'; import {ComponentFactory as ComponentFactoryR3} from '../render3/component_ref'; import {getComponentDef, getNgModuleDef} from '../render3/definition'; import {NgModuleFactory as NgModuleFactoryR3} from '../render3/ng_module_ref'; +import {maybeUnwrapFn} from '../render3/util/misc_utils'; import {ComponentFactory} from './component_factory'; import {NgModuleFactory} from './ng_module_factory'; @@ -60,11 +61,13 @@ export const Compiler_compileModuleAndAllComponentsSync__POST_R3__: (moduleTy ModuleWithComponentFactories { const ngModuleFactory = Compiler_compileModuleSync__POST_R3__(moduleType); const moduleDef = getNgModuleDef(moduleType) !; - const componentFactories = moduleDef.declarations.reduce((factories, declaration) => { - const componentDef = getComponentDef(declaration); - componentDef && factories.push(new ComponentFactoryR3(componentDef)); - return factories; - }, [] as ComponentFactory[]); + const componentFactories = + maybeUnwrapFn(moduleDef.declarations) + .reduce((factories: ComponentFactory[], declaration: Type) => { + const componentDef = getComponentDef(declaration); + componentDef && factories.push(new ComponentFactoryR3(componentDef)); + return factories; + }, [] as ComponentFactory[]); return new ModuleWithComponentFactories(ngModuleFactory, componentFactories); }; const Compiler_compileModuleAndAllComponentsSync = diff --git a/packages/core/src/metadata/ng_module.ts b/packages/core/src/metadata/ng_module.ts index c1b9312389..62780eec90 100644 --- a/packages/core/src/metadata/ng_module.ts +++ b/packages/core/src/metadata/ng_module.ts @@ -49,19 +49,19 @@ export interface NgModuleDef { type: T; /** List of components to bootstrap. */ - bootstrap: Type[]; + bootstrap: Type[]|(() => Type[]); /** List of components, directives, and pipes declared by this module. */ - declarations: Type[]; + declarations: Type[]|(() => Type[]); /** List of modules or `ModuleWithProviders` imported by this module. */ - imports: Type[]; + imports: Type[]|(() => Type[]); /** * List of modules, `ModuleWithProviders`, components, directives, or pipes exported by this * module. */ - exports: Type[]; + exports: Type[]|(() => Type[]); /** * Cached value of computed `transitiveCompileScopes` for this module. diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index 9831537ed6..faf86c541d 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/packages/core/src/render3/jit/module.ts @@ -19,7 +19,7 @@ import {getComponentDef, getDirectiveDef, getNgModuleDef, getPipeDef} from '../d import {NG_COMPONENT_DEF, NG_DIRECTIVE_DEF, NG_MODULE_DEF, NG_PIPE_DEF} from '../fields'; import {ComponentDef} from '../interfaces/definition'; import {NgModuleType} from '../ng_module_ref'; -import {renderStringify} from '../util/misc_utils'; +import {maybeUnwrapFn, renderStringify} from '../util/misc_utils'; import {angularCoreEnv} from './environment'; @@ -156,14 +156,17 @@ function verifySemanticsOfNgModuleDef(moduleType: NgModuleType): void { moduleType = resolveForwardRef(moduleType); const ngModuleDef = getNgModuleDef(moduleType, true); const errors: string[] = []; - ngModuleDef.declarations.forEach(verifyDeclarationsHaveDefinitions); + const declarations = maybeUnwrapFn(ngModuleDef.declarations); + const imports = maybeUnwrapFn(ngModuleDef.imports); + const exports = maybeUnwrapFn(ngModuleDef.exports); + declarations.forEach(verifyDeclarationsHaveDefinitions); const combinedDeclarations: Type[] = [ - ...ngModuleDef.declarations.map(resolveForwardRef), // - ...flatten(ngModuleDef.imports.map(computeCombinedExports), resolveForwardRef), + ...declarations.map(resolveForwardRef), // + ...flatten(imports.map(computeCombinedExports), resolveForwardRef), ]; - ngModuleDef.exports.forEach(verifyExportsAreDeclaredOrReExported); - ngModuleDef.declarations.forEach(verifyDeclarationIsUnique); - ngModuleDef.declarations.forEach(verifyComponentEntryComponentsIsPartOfNgModule); + exports.forEach(verifyExportsAreDeclaredOrReExported); + declarations.forEach(verifyDeclarationIsUnique); + declarations.forEach(verifyComponentEntryComponentsIsPartOfNgModule); const ngModule = getAnnotation(moduleType, 'NgModule'); if (ngModule) { @@ -297,15 +300,14 @@ export function resetCompiledComponents(): void { } /** - * Computes the combined declarations of explicit declarations, as well as declarations inherited - * by + * Computes the combined declarations of explicit declarations, as well as declarations inherited by * traversing the exports of imported modules. * @param type */ function computeCombinedExports(type: Type): Type[] { type = resolveForwardRef(type); const ngModuleDef = getNgModuleDef(type, true); - return [...flatten(ngModuleDef.exports.map((type) => { + return [...flatten(maybeUnwrapFn(ngModuleDef.exports).map((type) => { const ngModuleDef = getNgModuleDef(type); if (ngModuleDef) { verifySemanticsOfNgModuleDef(type as any as NgModuleType); @@ -388,7 +390,7 @@ export function transitiveScopesFor( }, }; - def.declarations.forEach(declared => { + maybeUnwrapFn(def.declarations).forEach(declared => { const declaredWithDefs = declared as Type& { ngPipeDef?: any; }; if (getPipeDef(declaredWithDefs)) { @@ -401,7 +403,7 @@ export function transitiveScopesFor( } }); - def.imports.forEach((imported: Type) => { + maybeUnwrapFn(def.imports).forEach((imported: Type) => { const importedType = imported as Type& { // If imported is an @NgModule: ngModuleDef?: NgModuleDef; @@ -422,7 +424,7 @@ export function transitiveScopesFor( importedScope.exported.pipes.forEach(entry => scopes.compilation.pipes.add(entry)); }); - def.exports.forEach((exported: Type) => { + maybeUnwrapFn(def.exports).forEach((exported: Type) => { const exportedType = exported as Type& { // Components, Directives, NgModules, and Pipes can all be exported. ngComponentDef?: any; diff --git a/packages/core/src/render3/ng_module_ref.ts b/packages/core/src/render3/ng_module_ref.ts index f13faac4d2..d445ded545 100644 --- a/packages/core/src/render3/ng_module_ref.ts +++ b/packages/core/src/render3/ng_module_ref.ts @@ -18,6 +18,7 @@ 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'; export interface NgModuleType extends Type { ngModuleDef: NgModuleDef; } @@ -43,7 +44,7 @@ export class NgModuleRef extends viewEngine_NgModuleRef implements Interna ngModuleDef, `NgModule '${stringify(ngModuleType)}' is not a subtype of 'NgModuleType'.`); - this._bootstrapComponents = ngModuleDef !.bootstrap; + this._bootstrapComponents = maybeUnwrapFn(ngModuleDef !.bootstrap); const additionalProviders: StaticProvider[] = [ { provide: viewEngine_NgModuleRef, diff --git a/packages/core/src/render3/util/misc_utils.ts b/packages/core/src/render3/util/misc_utils.ts index 829a2d16a6..0306adf7ed 100644 --- a/packages/core/src/render3/util/misc_utils.ts +++ b/packages/core/src/render3/util/misc_utils.ts @@ -76,3 +76,14 @@ export const INTERPOLATION_DELIMITER = `�`; export function isPropMetadataString(str: string): boolean { return str.indexOf(INTERPOLATION_DELIMITER) >= 0; } + +/** + * Unwrap a value which might be behind a closure (for forward declaration reasons). + */ +export function maybeUnwrapFn(value: T | (() => T)): T { + if (value instanceof Function) { + return value(); + } else { + return value; + } +} diff --git a/packages/core/test/render3/ivy/jit_spec.ts b/packages/core/test/render3/ivy/jit_spec.ts index 0e8e3c399c..f0cfaab740 100644 --- a/packages/core/test/render3/ivy/jit_spec.ts +++ b/packages/core/test/render3/ivy/jit_spec.ts @@ -158,6 +158,9 @@ ivyEnabled && describe('render3 jit', () => { const moduleDef: NgModuleDef = (Module as any).ngModuleDef; expect(moduleDef).toBeDefined(); + if (!Array.isArray(moduleDef.declarations)) { + return fail('Expected an array'); + } expect(moduleDef.declarations.length).toBe(1); expect(moduleDef.declarations[0]).toBe(Cmp); }); diff --git a/packages/core/testing/src/r3_test_bed.ts b/packages/core/testing/src/r3_test_bed.ts index 7b61642387..49f8989ea4 100644 --- a/packages/core/testing/src/r3_test_bed.ts +++ b/packages/core/testing/src/r3_test_bed.ts @@ -732,7 +732,7 @@ export class TestBedRender3 implements Injector, TestBed { * @internal */ _getComponentFactories(moduleType: NgModuleType): ComponentFactory[] { - return moduleType.ngModuleDef.declarations.reduce((factories, declaration) => { + return maybeUnwrapFn(moduleType.ngModuleDef.declarations).reduce((factories, declaration) => { const componentDef = (declaration as any).ngComponentDef; componentDef && factories.push(new ComponentFactory(componentDef, this._moduleRef)); return factories; @@ -820,3 +820,14 @@ class R3TestCompiler implements Compiler { class R3TestErrorHandler extends ErrorHandler { handleError(error: any) { throw error; } } + +/** + * Unwrap a value which might be behind a closure (for forward declaration reasons). + */ +function maybeUnwrapFn(value: T | (() => T)): T { + if (value instanceof Function) { + return value(); + } else { + return value; + } +}