From ec2be5dccbecf76ee01802a3157dfc22d54e5890 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Thu, 28 Sep 2017 09:39:16 -0700 Subject: [PATCH] fix(compiler): allow to use flat modules and summaries The combination of flat modules, flat module redirects and summaries lead to errors before. --- packages/compiler-cli/src/compiler_host.ts | 22 --- .../src/transformers/compiler_host.ts | 4 - packages/compiler-cli/test/ngc_spec.ts | 155 ++++++++++++------ .../src/aot/static_symbol_resolver.ts | 13 +- packages/compiler/src/aot/summary_resolver.ts | 13 +- .../compiler/src/aot/summary_serializer.ts | 32 ++-- packages/compiler/src/summary_resolver.ts | 2 + .../test/aot/static_symbol_resolver_spec.ts | 2 +- 8 files changed, 147 insertions(+), 96 deletions(-) diff --git a/packages/compiler-cli/src/compiler_host.ts b/packages/compiler-cli/src/compiler_host.ts index ac728009c9..2a0399b2d7 100644 --- a/packages/compiler-cli/src/compiler_host.ts +++ b/packages/compiler-cli/src/compiler_host.ts @@ -47,23 +47,6 @@ export abstract class BaseAotCompilerHost abstract getMetadataForSourceFile(filePath: string): ModuleMetadata|undefined; - protected getImportAs(fileName: string): string|undefined { - // Note: `importAs` can only be in .metadata.json files - // So it is enough to call this.readMetadata, and we get the - // benefit that this is cached. - if (DTS.test(fileName)) { - const metadatas = this.readMetadata(fileName); - if (metadatas) { - for (const metadata of metadatas) { - if (metadata.importAs) { - return metadata.importAs; - } - } - } - } - return undefined; - } - getMetadataFor(filePath: string): ModuleMetadata[]|undefined { if (!this.context.fileExists(filePath)) { // If the file doesn't exists then we cannot return metadata for the file. @@ -384,11 +367,6 @@ export class CompilerHost extends BaseAotCompilerHost { * NOTE: (*) the relative path is computed depending on `isGenDirChildOfRootDir`. */ fileNameToModuleName(importedFile: string, containingFile: string): string { - const importAs = this.getImportAs(importedFile); - if (importAs) { - return importAs; - } - // If a file does not yet exist (because we compile it later), we still need to // assume it exists it so that the `resolve` method works! if (importedFile !== containingFile && !this.context.fileExists(importedFile)) { diff --git a/packages/compiler-cli/src/transformers/compiler_host.ts b/packages/compiler-cli/src/transformers/compiler_host.ts index 51a3d82990..85e7702f4c 100644 --- a/packages/compiler-cli/src/transformers/compiler_host.ts +++ b/packages/compiler-cli/src/transformers/compiler_host.ts @@ -174,10 +174,6 @@ export class TsCompilerAotCompilerTypeCheckHostAdapter extends 'fileNameToModuleName from containingFile', containingFile, 'to importedFile', importedFile); } - const importAs = this.getImportAs(importedFile); - if (importAs) { - return importAs; - } // drop extension importedFile = importedFile.replace(EXT, ''); diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index a7fc5772f4..61cd86d2f6 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -833,60 +833,6 @@ describe('ngc transformer command-line', () => { shouldExist('index.metadata.json'); }); - it('should use the importAs for flat libraries instead of deep imports', () => { - // compile the flat module - writeFlatModule('index.js'); - expect(main(['-p', basePath], errorSpy)).toBe(0); - - // move the flat module output into node_modules - const flatModuleNodeModulesPath = path.resolve(basePath, 'node_modules', 'flat_module'); - fs.renameSync(outDir, flatModuleNodeModulesPath); - fs.renameSync( - path.resolve(basePath, 'src/flat.component.html'), - path.resolve(flatModuleNodeModulesPath, 'src/flat.component.html')); - // add a package.json - fs.writeFileSync( - path.resolve(flatModuleNodeModulesPath, 'package.json'), `{"typings": "./index.d.ts"}`); - - // and remove the sources. - fs.renameSync(path.resolve(basePath, 'src'), path.resolve(basePath, 'flat_module_src')); - fs.unlinkSync(path.resolve(basePath, 'public-api.ts')); - - writeConfig(` - { - "extends": "./tsconfig-base.json", - "files": ["index.ts"] - } - `); - write('index.ts', ` - import {NgModule} from '@angular/core'; - import {FlatModule} from 'flat_module'; - - @NgModule({ - imports: [FlatModule] - }) - export class MyModule {} - `); - - expect(main(['-p', basePath], errorSpy)).toBe(0); - - shouldExist('index.js'); - - const summary = - fs.readFileSync(path.resolve(basePath, 'built', 'index.ngsummary.json')).toString(); - // reference to the module itself - expect(summary).toMatch(/"filePath":"flat_module"/); - // no reference to a deep file - expect(summary).not.toMatch(/"filePath":"flat_module\//); - - const factory = - fs.readFileSync(path.resolve(basePath, 'built', 'index.ngfactory.js')).toString(); - // reference to the module itself - expect(factory).toMatch(/from "flat_module"/); - // no reference to a deep file - expect(factory).not.toMatch(/from "flat_module\//); - }); - describe('with tree example', () => { beforeEach(() => { writeConfig(); @@ -1050,6 +996,107 @@ describe('ngc transformer command-line', () => { outDir = path.resolve(basePath, 'built'); shouldExist('app/main.js'); }); + + it('shoud be able to compile libraries with summaries and flat modules', () => { + writeFiles(); + compile(); + + // libraries + // make `shouldExist` / `shouldNotExist` relative to `node_modules` + outDir = path.resolve(basePath, 'node_modules'); + shouldExist('flat_module/index.ngfactory.js'); + shouldExist('flat_module/index.ngsummary.json'); + + // app + // make `shouldExist` / `shouldNotExist` relative to `built` + outDir = path.resolve(basePath, 'built'); + shouldExist('app/main.ngfactory.js'); + + const factory = fs.readFileSync(path.resolve(outDir, 'app/main.ngfactory.js')).toString(); + // reference to the module itself + expect(factory).toMatch(/from "flat_module"/); + // no reference to a deep file + expect(factory).not.toMatch(/from "flat_module\//); + + function writeFiles() { + createFlatModuleInNodeModules(); + + // Angular + flat module + write('tsconfig-lib.json', `{ + "extends": "./tsconfig-base.json", + "angularCompilerOptions": { + "generateCodeForLibraries": true + }, + "compilerOptions": { + "outDir": "." + }, + "include": ["node_modules/@angular/core/**/*", "node_modules/flat_module/**/*"], + "exclude": [ + "node_modules/@angular/core/test/**", + "node_modules/@angular/core/testing/**" + ] + }`); + + // Application + write('app/tsconfig-app.json', `{ + "extends": "../tsconfig-base.json", + "angularCompilerOptions": { + "generateCodeForLibraries": false + }, + "compilerOptions": { + "rootDir": ".", + "outDir": "../built/app" + } + }`); + write('app/main.ts', ` + import {NgModule} from '@angular/core'; + import {FlatModule} from 'flat_module'; + + @NgModule({ + imports: [FlatModule] + }) + export class AppModule {} + `); + } + + function createFlatModuleInNodeModules() { + // compile the flat module + writeFlatModule('index.js'); + expect(main(['-p', basePath], errorSpy)).toBe(0); + + // move the flat module output into node_modules + const flatModuleNodeModulesPath = path.resolve(basePath, 'node_modules', 'flat_module'); + fs.renameSync(outDir, flatModuleNodeModulesPath); + fs.renameSync( + path.resolve(basePath, 'src/flat.component.html'), + path.resolve(flatModuleNodeModulesPath, 'src/flat.component.html')); + // and remove the sources. + fs.renameSync(path.resolve(basePath, 'src'), path.resolve(basePath, 'flat_module_src')); + fs.unlinkSync(path.resolve(basePath, 'public-api.ts')); + + // add a flatModuleIndexRedirect + write('node_modules/flat_module/redirect.metadata.json', `{ + "__symbolic": "module", + "version": 3, + "metadata": {}, + "exports": [ + { + "from": "./index" + } + ], + "flatModuleIndexRedirect": true, + "importAs": "flat_module" + }`); + write('node_modules/flat_module/redirect.d.ts', `export * from './index';`); + // add a package.json to use the redirect + write('node_modules/flat_module/package.json', `{"typings": "./redirect.d.ts"}`); + } + + function compile() { + expect(main(['-p', path.join(basePath, 'tsconfig-lib.json')], errorSpy)).toBe(0); + expect(main(['-p', path.join(basePath, 'app', 'tsconfig-app.json')], errorSpy)).toBe(0); + } + }); }); describe('expression lowering', () => { diff --git a/packages/compiler/src/aot/static_symbol_resolver.ts b/packages/compiler/src/aot/static_symbol_resolver.ts index b8c0991dec..39979ece99 100644 --- a/packages/compiler/src/aot/static_symbol_resolver.ts +++ b/packages/compiler/src/aot/static_symbol_resolver.ts @@ -164,10 +164,15 @@ export class StaticSymbolResolver { * Converts a file path to a module name that can be used as an `import`. */ fileNameToModuleName(importedFilePath: string, containingFilePath: string): string { - return this.knownFileNameToModuleNames.get(importedFilePath) || + return this.summaryResolver.getKnownModuleName(importedFilePath) || + this.knownFileNameToModuleNames.get(importedFilePath) || this.host.fileNameToModuleName(importedFilePath, containingFilePath); } + getKnownModuleName(filePath: string): string|null { + return this.knownFileNameToModuleNames.get(filePath) || null; + } + recordImportAs(sourceSymbol: StaticSymbol, targetSymbol: StaticSymbol) { sourceSymbol.assertNoMembers(); targetSymbol.assertNoMembers(); @@ -292,7 +297,11 @@ export class StaticSymbolResolver { this.resolvedFilePaths.add(filePath); const resolvedSymbols: ResolvedStaticSymbol[] = []; const metadata = this.getModuleMetadata(filePath); - + if (metadata['importAs']) { + // Index bundle indices should use the importAs module name defined + // in the bundle. + this.knownFileNameToModuleNames.set(filePath, metadata['importAs']); + } // handle the symbols in one of the re-export location if (metadata['exports']) { for (const moduleExport of metadata['exports']) { diff --git a/packages/compiler/src/aot/summary_resolver.ts b/packages/compiler/src/aot/summary_resolver.ts index 4999dd231b..4027d961f5 100644 --- a/packages/compiler/src/aot/summary_resolver.ts +++ b/packages/compiler/src/aot/summary_resolver.ts @@ -45,6 +45,7 @@ export class AotSummaryResolver implements SummaryResolver { private loadedFilePaths = new Map(); // Note: this will only contain StaticSymbols without members! private importAs = new Map(); + private knownFileNameToModuleNames = new Map(); constructor(private host: AotSummaryResolverHost, private staticSymbolCache: StaticSymbolCache) {} @@ -85,6 +86,13 @@ export class AotSummaryResolver implements SummaryResolver { return this.importAs.get(staticSymbol) !; } + /** + * Converts a file path to a module name that can be used as an `import`. + */ + getKnownModuleName(importedFilePath: string): string|null { + return this.knownFileNameToModuleNames.get(importedFilePath) || null; + } + addSummary(summary: Summary) { this.summaryCache.set(summary.symbol, summary); } private _loadSummaryFile(filePath: string): boolean { @@ -105,9 +113,12 @@ export class AotSummaryResolver implements SummaryResolver { hasSummary = json != null; this.loadedFilePaths.set(filePath, hasSummary); if (json) { - const {summaries, importAs} = + const {moduleName, summaries, importAs} = deserializeSummaries(this.staticSymbolCache, this, filePath, json); summaries.forEach((summary) => this.summaryCache.set(summary.symbol, summary)); + if (moduleName) { + this.knownFileNameToModuleNames.set(filePath, moduleName); + } importAs.forEach((importAs) => { this.importAs.set( importAs.symbol, diff --git a/packages/compiler/src/aot/summary_serializer.ts b/packages/compiler/src/aot/summary_serializer.ts index 8d6cfac961..43de1da9bf 100644 --- a/packages/compiler/src/aot/summary_serializer.ts +++ b/packages/compiler/src/aot/summary_serializer.ts @@ -21,7 +21,7 @@ export function serializeSummaries( metadata: CompileNgModuleMetadata | CompileDirectiveMetadata | CompilePipeMetadata | CompileTypeMetadata }[]): {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} { - const toJsonSerializer = new ToJsonSerializer(symbolResolver, summaryResolver); + const toJsonSerializer = new ToJsonSerializer(symbolResolver, summaryResolver, srcFileName); const forJitSerializer = new ForJitSerializer(forJitCtx, symbolResolver); // for symbols, we use everything except for the class metadata itself @@ -43,15 +43,18 @@ export function serializeSummaries( } }); - const {json, exportAs} = toJsonSerializer.serialize(srcFileName); + const {json, exportAs} = toJsonSerializer.serialize(); forJitSerializer.serialize(exportAs); return {json, exportAs}; } export function deserializeSummaries( symbolCache: StaticSymbolCache, summaryResolver: SummaryResolver, - libraryFileName: string, json: string): - {summaries: Summary[], importAs: {symbol: StaticSymbol, importAs: string}[]} { + libraryFileName: string, json: string): { + moduleName: string | null, + summaries: Summary[], + importAs: {symbol: StaticSymbol, importAs: string}[] +} { const deserializer = new FromJsonDeserializer(symbolCache, summaryResolver); return deserializer.deserialize(libraryFileName, json); } @@ -82,13 +85,15 @@ class ToJsonSerializer extends ValueTransformer { // StaticSymbols, but otherwise has the same shape as the original objects. private processedSummaryBySymbol = new Map(); private processedSummaries: any[] = []; + private moduleName: string|null; unprocessedSymbolSummariesBySymbol = new Map>(); constructor( private symbolResolver: StaticSymbolResolver, - private summaryResolver: SummaryResolver) { + private summaryResolver: SummaryResolver, private srcFileName: string) { super(); + this.moduleName = symbolResolver.getKnownModuleName(srcFileName); } addSummary(summary: Summary) { @@ -147,10 +152,10 @@ class ToJsonSerializer extends ValueTransformer { } } - serialize(srcFileName: string): - {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} { + serialize(): {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} { const exportAs: {symbol: StaticSymbol, exportAs: string}[] = []; const json = JSON.stringify({ + moduleName: this.moduleName, summaries: this.processedSummaries, symbols: this.symbols.map((symbol, index) => { symbol.assertNoMembers(); @@ -165,7 +170,7 @@ class ToJsonSerializer extends ValueTransformer { return { __symbol: index, name: symbol.name, - filePath: this.summaryResolver.toSummaryFileName(symbol.filePath, srcFileName), + filePath: this.summaryResolver.toSummaryFileName(symbol.filePath, this.srcFileName), importAs: importAs }; }) @@ -368,9 +373,12 @@ class FromJsonDeserializer extends ValueTransformer { super(); } - deserialize(libraryFileName: string, json: string): - {summaries: Summary[], importAs: {symbol: StaticSymbol, importAs: string}[]} { - const data: {summaries: any[], symbols: any[]} = JSON.parse(json); + deserialize(libraryFileName: string, json: string): { + moduleName: string | null, + summaries: Summary[], + importAs: {symbol: StaticSymbol, importAs: string}[] + } { + const data: {moduleName: string | null, summaries: any[], symbols: any[]} = JSON.parse(json); const importAs: {symbol: StaticSymbol, importAs: string}[] = []; this.symbols = []; data.symbols.forEach((serializedSymbol) => { @@ -383,7 +391,7 @@ class FromJsonDeserializer extends ValueTransformer { } }); const summaries = visitValue(data.summaries, this, null); - return {summaries, importAs}; + return {moduleName: data.moduleName, summaries, importAs}; } visitStringMap(map: {[key: string]: any}, context: any): any { diff --git a/packages/compiler/src/summary_resolver.ts b/packages/compiler/src/summary_resolver.ts index b7f8463a10..38d7dda3a0 100644 --- a/packages/compiler/src/summary_resolver.ts +++ b/packages/compiler/src/summary_resolver.ts @@ -21,6 +21,7 @@ export abstract class SummaryResolver { abstract resolveSummary(reference: T): Summary|null; abstract getSymbolsOf(filePath: string): T[]|null; abstract getImportAs(reference: T): T; + abstract getKnownModuleName(fileName: string): string|null; abstract addSummary(summary: Summary): void; } @@ -35,5 +36,6 @@ export class JitSummaryResolver implements SummaryResolver { } getSymbolsOf(): Type[] { return []; } getImportAs(reference: Type): Type { return reference; } + getKnownModuleName(fileName: string) { return null; } addSummary(summary: Summary) { this._summaries.set(summary.symbol, summary); } } diff --git a/packages/compiler/test/aot/static_symbol_resolver_spec.ts b/packages/compiler/test/aot/static_symbol_resolver_spec.ts index 92f9b162e5..d9ab7cd6b9 100644 --- a/packages/compiler/test/aot/static_symbol_resolver_spec.ts +++ b/packages/compiler/test/aot/static_symbol_resolver_spec.ts @@ -406,7 +406,7 @@ export class MockSummaryResolver implements SummaryResolver { const entry = this.importAs.find(entry => entry.symbol === symbol); return entry ? entry.importAs : undefined !; } - + getKnownModuleName(fileName: string): string|null { return null; } isLibraryFile(filePath: string): boolean { return filePath.endsWith('.d.ts'); } toSummaryFileName(filePath: string): string { return filePath.replace(/(\.d)?\.ts$/, '.d.ts'); } fromSummaryFileName(filePath: string): string { return filePath; }