From 8f109684337cd3a13494cbb1bbe523791bbd1048 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 19 May 2017 10:13:58 -0700 Subject: [PATCH] fix(compiler): compile `.ngfactory.ts` files even if nobody references them. This is especially important for library authors, as they will not reference the .ngfactory.ts files. Fixes #16741 --- build.sh | 2 +- .../hello_world__closure/tsconfig.json | 3 +- packages/compiler-cli/src/codegen.ts | 5 +- packages/compiler-cli/src/compiler_host.ts | 12 +- packages/compiler-cli/src/extract_i18n.ts | 2 +- packages/compiler-cli/src/extractor.ts | 3 +- packages/compiler-cli/src/ngtools_api.ts | 4 +- packages/compiler-cli/test/main_spec.ts | 103 ++++++++++++++++-- tools/@angular/tsc-wrapped/src/main.ts | 48 +++++--- tools/@angular/tsc-wrapped/src/tsc.ts | 43 +++----- 10 files changed, 166 insertions(+), 59 deletions(-) diff --git a/build.sh b/build.sh index 32e4c4f8d5b4a..c0c2e8d3d0102 100755 --- a/build.sh +++ b/build.sh @@ -266,7 +266,7 @@ compilePackage() { $NGC -p ${1}/tsconfig-build.json echo "====== Create ${1}/../${package_name}.d.ts re-export file for Closure" echo "$(cat ${LICENSE_BANNER}) ${N} export * from './${package_name}/index'" > ${2}/../${package_name}.d.ts - echo "{\"__symbolic\":\"module\",\"version\":3,\"metadata\":{},\"exports\":[{\"from\":\"./${package_name}/index\"}]}" > ${2}/../${package_name}.metadata.json + echo "{\"__symbolic\":\"module\",\"version\":3,\"metadata\":{},\"exports\":[{\"from\":\"./${package_name}/index\"}],\"bundleRedirect\":true}" > ${2}/../${package_name}.metadata.json fi for DIR in ${1}/* ; do diff --git a/integration/hello_world__closure/tsconfig.json b/integration/hello_world__closure/tsconfig.json index 0703f4d82ac37..68f4fd939751f 100644 --- a/integration/hello_world__closure/tsconfig.json +++ b/integration/hello_world__closure/tsconfig.json @@ -13,7 +13,8 @@ "noImplicitAny": false, "sourceMap": false, "experimentalDecorators": true, - "outDir": "built/src", + "outDir": "built", + "rootDir": ".", "declaration": true, "types": [] }, diff --git a/packages/compiler-cli/src/codegen.ts b/packages/compiler-cli/src/codegen.ts index 569ee37bd3b80..d27d055cc6273 100644 --- a/packages/compiler-cli/src/codegen.ts +++ b/packages/compiler-cli/src/codegen.ts @@ -36,17 +36,18 @@ export class CodeGenerator { public host: ts.CompilerHost, private compiler: compiler.AotCompiler, private ngCompilerHost: CompilerHost) {} - codegen(): Promise { + codegen(): Promise { return this.compiler .compileAll(this.program.getSourceFiles().map( sf => this.ngCompilerHost.getCanonicalFileName(sf.fileName))) .then(generatedModules => { - generatedModules.forEach(generatedModule => { + return generatedModules.map(generatedModule => { const sourceFile = this.program.getSourceFile(generatedModule.srcFileUrl); const emitPath = this.ngCompilerHost.calculateEmitPath(generatedModule.genFileUrl); const source = GENERATED_META_FILES.test(emitPath) ? generatedModule.source : generatedModule.source; this.host.writeFile(emitPath, source, false, () => {}, [sourceFile]); + return emitPath; }); }); } diff --git a/packages/compiler-cli/src/compiler_host.ts b/packages/compiler-cli/src/compiler_host.ts index bc14bbae02cc8..e9bc4e2a90d4d 100644 --- a/packages/compiler-cli/src/compiler_host.ts +++ b/packages/compiler-cli/src/compiler_host.ts @@ -33,6 +33,7 @@ export class CompilerHost implements AotCompilerHost { private resolverCache = new Map(); private bundleIndexCache = new Map(); private bundleIndexNames = new Set(); + private bundleRedirectNames = new Set(); private moduleFileNames = new Map(); protected resolveModuleNameHost: CompilerHostContext; @@ -280,7 +281,8 @@ export class CompilerHost implements AotCompilerHost { // Check for a bundle index. if (this.hasBundleIndex(filePath)) { const normalFilePath = path.normalize(filePath); - return this.bundleIndexNames.has(normalFilePath); + return this.bundleIndexNames.has(normalFilePath) || + this.bundleRedirectNames.has(normalFilePath); } } return true; @@ -331,7 +333,13 @@ export class CompilerHost implements AotCompilerHost { const metadataFile = typings.replace(DTS, '.metadata.json'); if (this.context.fileExists(metadataFile)) { const metadata = JSON.parse(this.context.readFile(metadataFile)); - if (metadata.importAs) { + if (metadata.bundleRedirect) { + this.bundleRedirectNames.add(typings); + // Note: don't set result = true, + // as this would mark this folder + // as having a bundleIndex too early without + // filling the bundleIndexNames. + } else if (metadata.importAs) { this.bundleIndexNames.add(typings); result = true; } diff --git a/packages/compiler-cli/src/extract_i18n.ts b/packages/compiler-cli/src/extract_i18n.ts index c8aec764a09ea..d5886e1b928a3 100644 --- a/packages/compiler-cli/src/extract_i18n.ts +++ b/packages/compiler-cli/src/extract_i18n.ts @@ -21,7 +21,7 @@ import {Extractor} from './extractor'; function extract( ngOptions: tsc.AngularCompilerOptions, cliOptions: tsc.I18nExtractionCliOptions, - program: ts.Program, host: ts.CompilerHost): Promise { + program: ts.Program, host: ts.CompilerHost) { return Extractor.create(ngOptions, program, host, cliOptions.locale) .extract(cliOptions.i18nFormat !, cliOptions.outFile); } diff --git a/packages/compiler-cli/src/extractor.ts b/packages/compiler-cli/src/extractor.ts index c8073b89c6643..aaf4a4f0779fc 100644 --- a/packages/compiler-cli/src/extractor.ts +++ b/packages/compiler-cli/src/extractor.ts @@ -27,7 +27,7 @@ export class Extractor { public host: ts.CompilerHost, private ngCompilerHost: CompilerHost, private program: ts.Program) {} - extract(formatName: string, outFile: string|null): Promise { + extract(formatName: string, outFile: string|null): Promise { // Checks the format and returns the extension const ext = this.getExtension(formatName); @@ -38,6 +38,7 @@ export class Extractor { const dstFile = outFile || `messages.${ext}`; const dstPath = path.join(this.options.genDir, dstFile); this.host.writeFile(dstPath, content, false); + return [dstPath]; }); } diff --git a/packages/compiler-cli/src/ngtools_api.ts b/packages/compiler-cli/src/ngtools_api.ts index 457278e8b4a1e..058b7948385cb 100644 --- a/packages/compiler-cli/src/ngtools_api.ts +++ b/packages/compiler-cli/src/ngtools_api.ts @@ -89,7 +89,7 @@ export class NgTools_InternalApi_NG_2 { * @internal * @private */ - static codeGen(options: NgTools_InternalApi_NG2_CodeGen_Options): Promise { + static codeGen(options: NgTools_InternalApi_NG2_CodeGen_Options): Promise { const hostContext: CompilerHostContext = new CustomLoaderModuleResolutionHostAdapter(options.readResource, options.host); const cliOptions: NgcCliOptions = { @@ -141,7 +141,7 @@ export class NgTools_InternalApi_NG_2 { * @internal * @private */ - static extractI18n(options: NgTools_InternalApi_NG2_ExtractI18n_Options): Promise { + static extractI18n(options: NgTools_InternalApi_NG2_ExtractI18n_Options): Promise { const hostContext: CompilerHostContext = new CustomLoaderModuleResolutionHostAdapter(options.readResource, options.host); diff --git a/packages/compiler-cli/test/main_spec.ts b/packages/compiler-cli/test/main_spec.ts index bbb50117be361..e7d0a98eab3be 100644 --- a/packages/compiler-cli/test/main_spec.ts +++ b/packages/compiler-cli/test/main_spec.ts @@ -13,39 +13,53 @@ import * as path from 'path'; import {main} from '../src/main'; +function getNgRootDir() { + const moduleFilename = module.filename.replace(/\\/g, '/'); + const distIndex = moduleFilename.indexOf('/dist/all'); + return moduleFilename.substr(0, distIndex); +} describe('compiler-cli', () => { let basePath: string; + let outDir: string; let write: (fileName: string, content: string) => void; + function writeConfig(tsconfig: string = '{"extends": "./tsconfig-base.json"}') { + write('tsconfig.json', tsconfig); + } + beforeEach(() => { basePath = makeTempDir(); write = (fileName: string, content: string) => { fs.writeFileSync(path.join(basePath, fileName), content, {encoding: 'utf-8'}); }; - write('tsconfig.json', `{ + write('tsconfig-base.json', `{ "compilerOptions": { "experimentalDecorators": true, "types": [], "outDir": "built", "declaration": true, "module": "es2015", - "moduleResolution": "node" - }, - "angularCompilerOptions": { - "annotateForClosureCompiler": true - }, - "files": ["test.ts"] + "moduleResolution": "node", + "lib": ["es6", "dom"] + } }`); + outDir = path.resolve(basePath, 'built'); + const ngRootDir = getNgRootDir(); const nodeModulesPath = path.resolve(basePath, 'node_modules'); fs.mkdirSync(nodeModulesPath); - fs.symlinkSync(path.resolve(__dirname, '..', '..'), path.resolve(nodeModulesPath, '@angular')); + fs.symlinkSync( + path.resolve(ngRootDir, 'dist', 'all', '@angular'), + path.resolve(nodeModulesPath, '@angular')); + fs.symlinkSync( + path.resolve(ngRootDir, 'node_modules', 'rxjs'), path.resolve(nodeModulesPath, 'rxjs')); }); // Restore reflector since AoT compiler will update it with a new static reflector afterEach(() => { ɵreflector.updateCapabilities(new ɵReflectionCapabilities()); }); it('should compile without errors', (done) => { + writeConfig(); write('test.ts', 'export const A = 1;'); const mockConsole = {error: (s: string) => {}}; @@ -62,6 +76,10 @@ describe('compiler-cli', () => { }); it('should not print the stack trace if user input file does not exist', (done) => { + writeConfig(`{ + "extends": "./tsconfig-base.json", + "files": ["test.ts"] + }`); const mockConsole = {error: (s: string) => {}}; spyOn(mockConsole, 'error'); @@ -79,6 +97,7 @@ describe('compiler-cli', () => { }); it('should not print the stack trace if user input file is malformed', (done) => { + writeConfig(); write('test.ts', 'foo;'); const mockConsole = {error: (s: string) => {}}; @@ -98,6 +117,7 @@ describe('compiler-cli', () => { }); it('should not print the stack trace if cannot find the imported module', (done) => { + writeConfig(); write('test.ts', `import {MyClass} from './not-exist-deps';`); const mockConsole = {error: (s: string) => {}}; @@ -118,6 +138,7 @@ describe('compiler-cli', () => { }); it('should not print the stack trace if cannot import', (done) => { + writeConfig(); write('empty-deps.ts', 'export const A = 1;'); write('test.ts', `import {MyClass} from './empty-deps';`); @@ -139,6 +160,7 @@ describe('compiler-cli', () => { }); it('should not print the stack trace if type mismatches', (done) => { + writeConfig(); write('empty-deps.ts', 'export const A = "abc";'); write('test.ts', ` import {A} from './empty-deps'; @@ -179,4 +201,69 @@ describe('compiler-cli', () => { }) .catch(e => done.fail(e)); }); + + describe('compile ngfactory files', () => { + it('should report errors for ngfactory files that are not referenced by root files', (done) => { + writeConfig(`{ + "extends": "./tsconfig-base.json", + "files": ["mymodule.ts"] + }`); + write('mymodule.ts', ` + import {NgModule, Component} from '@angular/core'; + + @Component({template: '{{unknownProp}}'}) + export class MyComp {} + + @NgModule({declarations: [MyComp]}) + export class MyModule {} + `); + + const mockConsole = {error: (s: string) => {}}; + + const errorSpy = spyOn(mockConsole, 'error'); + + main({p: basePath}, mockConsole.error) + .then((exitCode) => { + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(errorSpy.calls.mostRecent().args[0]) + .toContain('Error at ' + path.join(basePath, 'mymodule.ngfactory.ts')); + expect(errorSpy.calls.mostRecent().args[0]) + .toContain(`Property 'unknownProp' does not exist on type 'MyComp'`); + + expect(exitCode).toEqual(1); + done(); + }) + .catch(e => done.fail(e)); + }); + + it('should compile ngfactory files that are not referenced by root files', (done) => { + writeConfig(`{ + "extends": "./tsconfig-base.json", + "files": ["mymodule.ts"] + }`); + write('mymodule.ts', ` + import {CommonModule} from '@angular/common'; + import {NgModule} from '@angular/core'; + + @NgModule({ + imports: [CommonModule] + }) + export class MyModule {} + `); + + main({p: basePath}) + .then((exitCode) => { + expect(exitCode).toEqual(0); + + expect(fs.existsSync(path.resolve(outDir, 'mymodule.ngfactory.js'))).toBe(true); + expect(fs.existsSync(path.resolve( + outDir, 'node_modules', '@angular', 'core', 'src', + 'application_module.ngfactory.js'))) + .toBe(true); + + done(); + }) + .catch(e => done.fail(e)); + }); + }); }); diff --git a/tools/@angular/tsc-wrapped/src/main.ts b/tools/@angular/tsc-wrapped/src/main.ts index 82455b1409195..5a5ca27a4d256 100644 --- a/tools/@angular/tsc-wrapped/src/main.ts +++ b/tools/@angular/tsc-wrapped/src/main.ts @@ -11,22 +11,27 @@ import * as path from 'path'; import * as tsickle from 'tsickle'; import * as ts from 'typescript'; -import {check, tsc} from './tsc'; - -import NgOptions from './options'; -import {MetadataWriterHost, SyntheticIndexHost} from './compiler_host'; +import {CompilerHostAdapter, MetadataBundler} from './bundler'; import {CliOptions} from './cli_options'; -import {VinylFile, isVinylFile} from './vinyl_file'; -import {MetadataBundler, CompilerHostAdapter} from './bundler'; +import {MetadataWriterHost, SyntheticIndexHost} from './compiler_host'; import {privateEntriesToIndex} from './index_writer'; +import NgOptions from './options'; +import {check, tsc} from './tsc'; +import {isVinylFile, VinylFile} from './vinyl_file'; + export {UserError} from './tsc'; const DTS = /\.d\.ts$/; const JS_EXT = /(\.js|)$/; - -export type CodegenExtension = - (ngOptions: NgOptions, cliOptions: CliOptions, program: ts.Program, host: ts.CompilerHost) => - Promise; +const TS_EXT = /\.ts$/; + +export interface CodegenExtension { + /** + * Returns the generated file names. + */ + (ngOptions: NgOptions, cliOptions: CliOptions, program: ts.Program, + host: ts.CompilerHost): Promise; +} export function main( project: string | VinylFile, cliOptions: CliOptions, codegen?: CodegenExtension, @@ -46,10 +51,18 @@ export function main( const basePath = path.resolve(process.cwd(), cliOptions.basePath || projectDir); // read the configuration options from wherever you store them - const {parsed, ngOptions} = tsc.readConfiguration(project, basePath, options); + let {parsed, ngOptions} = tsc.readConfiguration(project, basePath, options); ngOptions.basePath = basePath; - const createProgram = (host: ts.CompilerHost, oldProgram?: ts.Program) => - ts.createProgram(parsed.fileNames, parsed.options, host, oldProgram); + let rootFileNames: string[] = parsed.fileNames.slice(0); + const createProgram = (host: ts.CompilerHost, oldProgram?: ts.Program) => { + return ts.createProgram(rootFileNames.slice(0), parsed.options, host, oldProgram); + }; + const addGeneratedFileName = (genFileName: string) => { + if (genFileName.startsWith(basePath) && TS_EXT.exec(genFileName)) { + rootFileNames.push(genFileName); + } + }; + const diagnostics = (parsed.options as any).diagnostics; if (diagnostics) (ts as any).performance.enable(); @@ -83,7 +96,7 @@ export function main( const libraryIndex = `./${path.basename(indexModule)}`; const content = privateEntriesToIndex(libraryIndex, metadataBundle.privates); host = new SyntheticIndexHost(host, {name, content, metadata}); - parsed.fileNames.push(name); + addGeneratedFileName(name); } const tsickleCompilerHostOptions: tsickle.Options = { @@ -109,12 +122,14 @@ export function main( check(errors); if (ngOptions.skipTemplateCodegen || !codegen) { - codegen = () => Promise.resolve(null); + codegen = () => Promise.resolve([]); } if (diagnostics) console.time('NG codegen'); - return codegen(ngOptions, cliOptions, program, host).then(() => { + return codegen(ngOptions, cliOptions, program, host).then((genFiles) => { if (diagnostics) console.timeEnd('NG codegen'); + // Add the generated files to the configuration so they will become part of the program. + genFiles.forEach(genFileName => addGeneratedFileName(genFileName)); let definitionsHost: ts.CompilerHost = tsickleCompilerHost; if (!ngOptions.skipMetadataEmit) { // if tsickle is not not used for emitting, but we do use the MetadataWriterHost, @@ -123,6 +138,7 @@ export function main( ngOptions.annotationsAs === 'decorators' && !ngOptions.annotateForClosureCompiler; definitionsHost = new MetadataWriterHost(tsickleCompilerHost, ngOptions, emitJsFiles); } + // Create a new program since codegen files were created after making the old program let programWithCodegen = createProgram(definitionsHost, program); tsc.typeCheck(host, programWithCodegen); diff --git a/tools/@angular/tsc-wrapped/src/tsc.ts b/tools/@angular/tsc-wrapped/src/tsc.ts index 96889c943954f..15295dfe55629 100644 --- a/tools/@angular/tsc-wrapped/src/tsc.ts +++ b/tools/@angular/tsc-wrapped/src/tsc.ts @@ -103,16 +103,19 @@ export function validateAngularCompilerOptions(options: AngularCompilerOptions): } export class Tsc implements CompilerInterface { - public ngOptions: AngularCompilerOptions; - public parsed: ts.ParsedCommandLine; - private basePath: string; + private parseConfigHost: ts.ParseConfigHost; - constructor(private readFile = ts.sys.readFile, private readDirectory = ts.sys.readDirectory) {} + constructor(private readFile = ts.sys.readFile, private readDirectory = ts.sys.readDirectory) { + this.parseConfigHost = { + useCaseSensitiveFileNames: true, + fileExists: existsSync, + readDirectory: this.readDirectory, + readFile: ts.sys.readFile + }; + } readConfiguration( project: string|VinylFile, basePath: string, existingOptions?: ts.CompilerOptions) { - this.basePath = basePath; - // Allow a directory containing tsconfig.json as the project value // Note, TS@next returns an empty array, while earlier versions throw try { @@ -135,31 +138,21 @@ export class Tsc implements CompilerInterface { })(); check([error]); - // Do not inline `host` into `parseJsonConfigFileContent` until after - // g3 is updated to the latest TypeScript. - // The issue is that old typescript only has `readDirectory` while - // the newer TypeScript has additional `useCaseSensitiveFileNames` and - // `fileExists`. Inlining will trigger an error of extra parameters. - const host = { - useCaseSensitiveFileNames: true, - fileExists: existsSync, - readDirectory: this.readDirectory, - readFile: ts.sys.readFile - }; - this.parsed = ts.parseJsonConfigFileContent(config, host, basePath, existingOptions); + const parsed = + ts.parseJsonConfigFileContent(config, this.parseConfigHost, basePath, existingOptions); - check(this.parsed.errors); + check(parsed.errors); // Default codegen goes to the current directory // Parsed options are already converted to absolute paths - this.ngOptions = config.angularCompilerOptions || {}; - this.ngOptions.genDir = path.join(basePath, this.ngOptions.genDir || '.'); - for (const key of Object.keys(this.parsed.options)) { - this.ngOptions[key] = this.parsed.options[key]; + const ngOptions = config.angularCompilerOptions || {}; + ngOptions.genDir = path.join(basePath, ngOptions.genDir || '.'); + for (const key of Object.keys(parsed.options)) { + ngOptions[key] = parsed.options[key]; } - check(validateAngularCompilerOptions(this.ngOptions)); + check(validateAngularCompilerOptions(ngOptions)); - return {parsed: this.parsed, ngOptions: this.ngOptions}; + return {parsed, ngOptions}; } typeCheck(compilerHost: ts.CompilerHost, program: ts.Program): void {