From c8c8648abf45b57a3bfe26d6e02034197d1e24e8 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 11 Nov 2018 19:16:04 +0100 Subject: [PATCH] =?UTF-8?q?fix(ivy):=20prevent=20ngcc=20from=20referencing?= =?UTF-8?q?=20missing=20=C9=B5setClassMetadata=20(#27055)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When ngtsc compiles @angular/core, it rewrites core imports to the r3_symbols.ts file that exposes all internal symbols under their external name. When creating the FESM bundle, the r3_symbols.ts file causes the external symbol names to be rewritten to their internal name. Under ngcc compilations of FESM bundles, the indirection of r3_symbols.ts is no longer in place such that the external names are retained in the bundle. Previously, the external name `ɵdefineNgModule` was explicitly declared internally to resolve this issue, but the recently added `setClassMetadata` was not declared as such, causing runtime errors. Instead of relying on the r3_symbols.ts file to perform the rewrite of the external modules to their internal variants, the translation is moved into the `ImportManager` during the compilation itself. This avoids the need for providing the external name manually. PR Close #27055 --- integration/ngcc/test.sh | 8 +- .../ngcc/src/rendering/ngcc_import_manager.ts | 7 +- .../src/ngcc/test/rendering/renderer_spec.ts | 79 +++++++++++++++---- .../src/ngtsc/metadata/src/resolver.ts | 2 +- .../src/ngtsc/translator/src/translator.ts | 56 ++++++++----- .../compiler-cli/src/ngtsc/util/src/path.ts | 6 +- .../core/src/core_render3_private_export.ts | 5 +- packages/core/src/r3_symbols.ts | 13 +-- .../hello_world_r2/bundle.golden_symbols.json | 3 - .../todo_r2/bundle.golden_symbols.json | 3 - 10 files changed, 121 insertions(+), 61 deletions(-) diff --git a/integration/ngcc/test.sh b/integration/ngcc/test.sh index c88adbb4ac..d46ad3201d 100755 --- a/integration/ngcc/test.sh +++ b/integration/ngcc/test.sh @@ -24,13 +24,13 @@ ivy-ngcc if [[ $? != 0 ]]; then exit 1; fi # Did it compile @angular/core/ApplicationModule correctly? - grep "ApplicationModule.ngModuleDef = ɵdefineNgModule" node_modules/@angular/core/fesm2015/core.js + grep "ApplicationModule.ngModuleDef = defineNgModule" node_modules/@angular/core/fesm2015/core.js if [[ $? != 0 ]]; then exit 1; fi - grep "ApplicationModule.ngModuleDef = ɵdefineNgModule" node_modules/@angular/core/fesm5/core.js + grep "ApplicationModule.ngModuleDef = defineNgModule" node_modules/@angular/core/fesm5/core.js if [[ $? != 0 ]]; then exit 1; fi - grep "ApplicationModule.ngModuleDef = ɵngcc0.ɵdefineNgModule" node_modules/@angular/core/esm2015/src/application_module.js + grep "ApplicationModule.ngModuleDef = ɵngcc0.defineNgModule" node_modules/@angular/core/esm2015/src/application_module.js if [[ $? != 0 ]]; then exit 1; fi - grep "ApplicationModule.ngModuleDef = ɵngcc0.ɵdefineNgModule" node_modules/@angular/core/esm5/src/application_module.js + grep "ApplicationModule.ngModuleDef = ɵngcc0.defineNgModule" node_modules/@angular/core/esm5/src/application_module.js if [[ $? != 0 ]]; then exit 1; fi # Can it be safely run again (as a noop)? diff --git a/packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_manager.ts b/packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_manager.ts index e8d3aa05a0..2dbd05f1d8 100644 --- a/packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_manager.ts +++ b/packages/compiler-cli/src/ngcc/src/rendering/ngcc_import_manager.ts @@ -12,10 +12,11 @@ import {ImportManager} from '../../../ngtsc/translator'; export class NgccImportManager extends ImportManager { constructor(private isFlat: boolean, isCore: boolean, prefix?: string) { super(isCore, prefix); } - generateNamedImport(moduleName: string, symbol: string): string|null { + generateNamedImport(moduleName: string, symbol: string): + {moduleImport: string | null, symbol: string} { if (this.isFlat && this.isCore && moduleName === '@angular/core') { - return null; + return {moduleImport: null, symbol: this.rewriteSymbol(moduleName, symbol)}; } return super.generateNamedImport(moduleName, symbol); } -} \ No newline at end of file +} diff --git a/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts b/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts index 3b30a01600..1fc8c6da8e 100644 --- a/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts @@ -17,7 +17,10 @@ import {Esm2015ReflectionHost} from '../../src/host/esm2015_host'; import {Renderer} from '../../src/rendering/renderer'; class TestRenderer extends Renderer { - constructor(host: Esm2015ReflectionHost) { super(host, false, null, '/src', '/dist', false); } + constructor( + host: Esm2015ReflectionHost, isCore: boolean, rewriteCoreImportsTo: ts.SourceFile|null) { + super(host, isCore, rewriteCoreImportsTo, '/src', '/dist', false); + } addImports(output: MagicString, imports: {name: string, as: string}[]) { output.prepend('\n// ADD IMPORTS\n'); } @@ -35,13 +38,18 @@ class TestRenderer extends Renderer { } } -function createTestRenderer(file: {name: string, contents: string}) { - const program = makeProgram(file); - const host = new Esm2015ReflectionHost(false, program.getTypeChecker()); +function createTestRenderer( + files: {name: string, contents: string}[], + options: {isCore?: boolean, rewriteCoreImportsTo?: string} = {}) { + const program = makeProgram(...files); + const host = new Esm2015ReflectionHost(options.isCore || false, program.getTypeChecker()); const decorationAnalyses = - new DecorationAnalyzer(program.getTypeChecker(), host, [''], false).analyzeProgram(program); + new DecorationAnalyzer(program.getTypeChecker(), host, [''], options.isCore || false) + .analyzeProgram(program); const switchMarkerAnalyses = new SwitchMarkerAnalyzer(host).analyzeProgram(program); - const renderer = new TestRenderer(host); + const rewriteCoreImportsTo = + options.rewriteCoreImportsTo ? program.getSourceFile(options.rewriteCoreImportsTo) ! : null; + const renderer = new TestRenderer(host, options.isCore || false, rewriteCoreImportsTo); spyOn(renderer, 'addImports').and.callThrough(); spyOn(renderer, 'addDefinitions').and.callThrough(); spyOn(renderer, 'removeDecorators').and.callThrough(); @@ -94,7 +102,7 @@ describe('Renderer', () => { it('should render the modified contents; and a new map file, if the original provided no map file.', () => { const {renderer, program, decorationAnalyses, switchMarkerAnalyses} = - createTestRenderer(INPUT_PROGRAM); + createTestRenderer([INPUT_PROGRAM]); const result = renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); expect(result[0].path).toEqual('/dist/file.js'); expect(result[0].contents) @@ -106,7 +114,7 @@ describe('Renderer', () => { it('should call addImports with the source code and info about the core Angular library.', () => { const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = - createTestRenderer(INPUT_PROGRAM); + createTestRenderer([INPUT_PROGRAM]); renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); const addImportsSpy = renderer.addImports as jasmine.Spy; expect(addImportsSpy.calls.first().args[0].toString()).toEqual(RENDERED_CONTENTS); @@ -118,7 +126,7 @@ describe('Renderer', () => { it('should call addDefinitions with the source code, the analyzed class and the renderered definitions.', () => { const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = - createTestRenderer(INPUT_PROGRAM); + createTestRenderer([INPUT_PROGRAM]); renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); const addDefinitionsSpy = renderer.addDefinitions as jasmine.Spy; expect(addDefinitionsSpy.calls.first().args[0].toString()).toEqual(RENDERED_CONTENTS); @@ -137,7 +145,7 @@ A.ngDirectiveDef = ɵngcc0.ɵdefineDirective({ type: A, selectors: [["", "a", "" it('should call removeDecorators with the source code, a map of class decorators that have been analyzed', () => { const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = - createTestRenderer(INPUT_PROGRAM); + createTestRenderer([INPUT_PROGRAM]); renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); const removeDecoratorsSpy = renderer.removeDecorators as jasmine.Spy; expect(removeDecoratorsSpy.calls.first().args[0].toString()).toEqual(RENDERED_CONTENTS); @@ -157,10 +165,10 @@ A.ngDirectiveDef = ɵngcc0.ɵdefineDirective({ type: A, selectors: [["", "a", "" it('should merge any inline source map from the original file and write the output as an inline source map', () => { - const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = createTestRenderer({ + const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = createTestRenderer([{ ...INPUT_PROGRAM, contents: INPUT_PROGRAM.contents + '\n' + INPUT_PROGRAM_MAP.toComment() - }); + }]); const result = renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); expect(result[0].path).toEqual('/dist/file.js'); expect(result[0].contents) @@ -172,10 +180,10 @@ A.ngDirectiveDef = ɵngcc0.ɵdefineDirective({ type: A, selectors: [["", "a", "" () => { // Mock out reading the map file from disk spyOn(fs, 'readFileSync').and.returnValue(INPUT_PROGRAM_MAP.toJSON()); - const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = createTestRenderer({ + const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = createTestRenderer([{ ...INPUT_PROGRAM, contents: INPUT_PROGRAM.contents + '\n//# sourceMappingURL=file.js.map' - }); + }]); const result = renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); expect(result[0].path).toEqual('/dist/file.js'); expect(result[0].contents) @@ -183,5 +191,48 @@ A.ngDirectiveDef = ɵngcc0.ɵdefineDirective({ type: A, selectors: [["", "a", "" expect(result[1].path).toEqual('/dist/file.js.map'); expect(result[1].contents).toEqual(MERGED_OUTPUT_PROGRAM_MAP.toJSON()); }); + + describe('@angular/core support', () => { + + it('should render relative imports in ESM bundles', () => { + const R3_SYMBOLS_FILE = { + name: '/src/r3_symbols.js', + contents: `export const NgModule = () => null;` + }; + const CORE_FILE = { + name: '/src/core.js', + contents: + `import { NgModule } from './ng_module';\nexport class MyModule {}\nMyModule.decorators = [\n { type: NgModule, args: [] }\n];\n` + }; + + const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = createTestRenderer( + [R3_SYMBOLS_FILE, CORE_FILE], + {isCore: true, rewriteCoreImportsTo: R3_SYMBOLS_FILE.name}); + renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); + const addDefinitionsSpy = renderer.addDefinitions as jasmine.Spy; + expect(addDefinitionsSpy.calls.first().args[2]) + .toContain(`/*@__PURE__*/ ɵngcc0.setClassMetadata(`); + const addImportsSpy = renderer.addImports as jasmine.Spy; + expect(addImportsSpy.calls.first().args[1]).toEqual([{name: './r3_symbols', as: 'ɵngcc0'}]); + }); + + it('should render no imports in FESM bundles', () => { + const CORE_FILE = { + name: '/src/core.js', + contents: `export const NgModule = () => null; + export class MyModule {}\nMyModule.decorators = [\n { type: NgModule, args: [] }\n];\n` + }; + + const {decorationAnalyses, program, renderer, switchMarkerAnalyses} = + createTestRenderer([CORE_FILE], {isCore: true}); + renderer.renderProgram(program, decorationAnalyses, switchMarkerAnalyses); + const addDefinitionsSpy = renderer.addDefinitions as jasmine.Spy; + expect(addDefinitionsSpy.calls.first().args[2]) + .toContain(`/*@__PURE__*/ setClassMetadata(`); + const addImportsSpy = renderer.addImports as jasmine.Spy; + expect(addImportsSpy.calls.first().args[1]).toEqual([]); + }); + + }); }); }); diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts b/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts index 60675469ea..7c5d0e7889 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts @@ -17,7 +17,7 @@ import * as ts from 'typescript'; import {ClassMemberKind, ReflectionHost} from '../../host'; -const TS_DTS_JS_EXTENSION = /(\.d)?\.ts|\.js$/; +const TS_DTS_JS_EXTENSION = /(?:\.d)?\.ts$|\.js$/; /** * Represents a value which cannot be determined statically. diff --git a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts index cb9eddd0ee..e24d0d69f4 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts @@ -38,16 +38,16 @@ const BINARY_OPERATORS = new Map([ [BinaryOperator.Plus, ts.SyntaxKind.PlusToken], ]); -const CORE_SUPPORTED_SYMBOLS = new Set([ - 'defineInjectable', - 'defineInjector', - 'ɵdefineNgModule', - 'inject', - 'ɵsetClassMetadata', - 'ɵInjectableDef', - 'ɵInjectorDef', - 'ɵNgModuleDefWithMeta', - 'ɵNgModuleFactory', +const CORE_SUPPORTED_SYMBOLS = new Map([ + ['defineInjectable', 'defineInjectable'], + ['defineInjector', 'defineInjector'], + ['ɵdefineNgModule', 'defineNgModule'], + ['inject', 'inject'], + ['ɵsetClassMetadata', 'setClassMetadata'], + ['ɵInjectableDef', 'InjectableDef'], + ['ɵInjectorDef', 'InjectorDef'], + ['ɵNgModuleDefWithMeta', 'NgModuleDefWithMeta'], + ['ɵNgModuleFactory', 'NgModuleFactory'], ]); export class ImportManager { @@ -56,14 +56,28 @@ export class ImportManager { constructor(protected isCore: boolean, private prefix = 'i') {} - generateNamedImport(moduleName: string, symbol: string): string|null { + generateNamedImport(moduleName: string, symbol: string): + {moduleImport: string | null, symbol: string} { if (!this.moduleToIndex.has(moduleName)) { this.moduleToIndex.set(moduleName, `${this.prefix}${this.nextIndex++}`); } - if (this.isCore && moduleName === '@angular/core' && !CORE_SUPPORTED_SYMBOLS.has(symbol)) { - throw new Error(`Importing unexpected symbol ${symbol} while compiling core`); + + return { + moduleImport: this.moduleToIndex.get(moduleName) !, + symbol: this.rewriteSymbol(moduleName, symbol) + }; + } + + protected rewriteSymbol(moduleName: string, symbol: string): string { + if (this.isCore && moduleName === '@angular/core') { + if (!CORE_SUPPORTED_SYMBOLS.has(symbol)) { + throw new Error(`Importing unexpected symbol ${symbol} while compiling core`); + } + + symbol = CORE_SUPPORTED_SYMBOLS.get(symbol) !; } - return this.moduleToIndex.get(moduleName) !; + + return symbol; } getAllImports(contextPath: string, rewriteCoreImportsTo: ts.SourceFile|null): @@ -216,12 +230,13 @@ class ExpressionTranslatorVisitor implements ExpressionVisitor, StatementVisitor if (ast.value.moduleName === null || ast.value.name === null) { throw new Error(`Import unknown module or symbol ${ast.value}`); } - const importIdentifier = this.imports.generateNamedImport(ast.value.moduleName, ast.value.name); - if (importIdentifier === null) { - return ts.createIdentifier(ast.value.name); + const {moduleImport, symbol} = + this.imports.generateNamedImport(ast.value.moduleName, ast.value.name); + if (moduleImport === null) { + return ts.createIdentifier(symbol); } else { return ts.createPropertyAccess( - ts.createIdentifier(importIdentifier), ts.createIdentifier(ast.value.name)); + ts.createIdentifier(moduleImport), ts.createIdentifier(symbol)); } } @@ -382,8 +397,9 @@ export class TypeTranslatorVisitor implements ExpressionVisitor, TypeVisitor { if (ast.value.moduleName === null || ast.value.name === null) { throw new Error(`Import unknown module or symbol`); } - const moduleSymbol = this.imports.generateNamedImport(ast.value.moduleName, ast.value.name); - const base = `${moduleSymbol}.${ast.value.name}`; + const {moduleImport, symbol} = + this.imports.generateNamedImport(ast.value.moduleName, ast.value.name); + const base = moduleImport ? `${moduleImport}.${symbol}` : symbol; if (ast.typeParams !== null) { const generics = ast.typeParams.map(type => type.visitType(this, context)).join(', '); return `${base}<${generics}>`; diff --git a/packages/compiler-cli/src/ngtsc/util/src/path.ts b/packages/compiler-cli/src/ngtsc/util/src/path.ts index 954eba6777..4c5d11c55f 100644 --- a/packages/compiler-cli/src/ngtsc/util/src/path.ts +++ b/packages/compiler-cli/src/ngtsc/util/src/path.ts @@ -10,10 +10,10 @@ import * as path from 'path'; -const TS_DTS_EXTENSION = /(\.d)?\.ts$/; +const TS_DTS_JS_EXTENSION = /(?:\.d)?\.ts$|\.js$/; export function relativePathBetween(from: string, to: string): string|null { - let relative = path.posix.relative(path.dirname(from), to).replace(TS_DTS_EXTENSION, ''); + let relative = path.posix.relative(path.dirname(from), to).replace(TS_DTS_JS_EXTENSION, ''); if (relative === '') { return null; @@ -25,4 +25,4 @@ export function relativePathBetween(from: string, to: string): string|null { } return relative; -} \ No newline at end of file +} diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts index 1343f86cc4..8f815a5faa 100644 --- a/packages/core/src/core_render3_private_export.ts +++ b/packages/core/src/core_render3_private_export.ts @@ -7,15 +7,12 @@ */ // clang-format off -// We need to have `ɵdefineNgModule` defined locally for flat-file ngcc compilation. -// More details in the commit where this is added. -import {defineNgModule} from './render3/index'; -export const ɵdefineNgModule = defineNgModule; export { defineBase as ɵdefineBase, defineComponent as ɵdefineComponent, defineDirective as ɵdefineDirective, definePipe as ɵdefinePipe, + defineNgModule as ɵdefineNgModule, detectChanges as ɵdetectChanges, renderComponent as ɵrenderComponent, ComponentType as ɵComponentType, diff --git a/packages/core/src/r3_symbols.ts b/packages/core/src/r3_symbols.ts index b5c702c389..2bddd57e02 100644 --- a/packages/core/src/r3_symbols.ts +++ b/packages/core/src/r3_symbols.ts @@ -14,17 +14,18 @@ * compiler writes imports to this file. * * Only a subset of such imports are supported - core is not allowed to declare components or pipes. - * A check in ngtsc's translator.ts validates this condition. + * A check in ngtsc's translator.ts validates this condition. The translator is responsible for + * translating an external name (prefixed with ɵ) to the internal symbol name as exported below. * * The below symbols are used for @Injectable and @NgModule compilation. */ -export {InjectableDef as ɵInjectableDef, InjectorDef as ɵInjectorDef, defineInjectable, defineInjector} from './di/defs'; +export {InjectableDef, InjectorDef, defineInjectable, defineInjector} from './di/defs'; export {inject} from './di/injector_compatibility'; -export {NgModuleDef as ɵNgModuleDef, NgModuleDefWithMeta as ɵNgModuleDefWithMeta} from './metadata/ng_module'; -export {defineNgModule as ɵdefineNgModule} from './render3/definition'; -export {setClassMetadata as ɵsetClassMetadata} from './render3/metadata'; -export {NgModuleFactory as ɵNgModuleFactory} from './render3/ng_module_ref'; +export {NgModuleDef, NgModuleDefWithMeta} from './metadata/ng_module'; +export {defineNgModule} from './render3/definition'; +export {setClassMetadata} from './render3/metadata'; +export {NgModuleFactory} from './render3/ng_module_ref'; /** diff --git a/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json index 9c4fe6aecf..84bb286881 100644 --- a/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world_r2/bundle.golden_symbols.json @@ -1420,8 +1420,5 @@ }, { "name": "wtfLeave" - }, - { - "name": "ɵdefineNgModule" } ] \ No newline at end of file diff --git a/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json b/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json index bf92044df9..6e0a529556 100644 --- a/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo_r2/bundle.golden_symbols.json @@ -2704,8 +2704,5 @@ }, { "name": "wtfLeave" - }, - { - "name": "ɵdefineNgModule" } ] \ No newline at end of file