From d28697e499cfc6c07c2cf73cd6ed9c1fbde7d4ae Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Wed, 25 Oct 2017 16:51:12 -0700 Subject: [PATCH 1/4] =?UTF-8?q?fix(compiler):=20don=E2=80=99t=20store=20in?= =?UTF-8?q?valid=20state=20when=20using=20`listLazyRoutes`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, `listLazyRoute` would store invalid information in a compiler internal cache, which lead to incorrect paths that were used during emit. This commit fixes this. --- .../test/transformers/program_spec.ts | 28 +++++++++++++++++++ packages/compiler/src/aot/static_reflector.ts | 6 ++-- .../test/aot/static_reflector_spec.ts | 16 +++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/packages/compiler-cli/test/transformers/program_spec.ts b/packages/compiler-cli/test/transformers/program_spec.ts index b74a49c3d9f58..19c945d101cdd 100644 --- a/packages/compiler-cli/test/transformers/program_spec.ts +++ b/packages/compiler-cli/test/transformers/program_spec.ts @@ -617,6 +617,34 @@ describe('ng program', () => { ]); }); + it('should emit correctly after listing lazyRoutes', () => { + testSupport.writeFiles({ + 'src/main.ts': ` + import {NgModule} from '@angular/core'; + import {RouterModule} from '@angular/router'; + + @NgModule({ + imports: [RouterModule.forRoot([{loadChildren: './lazy/lazy#LazyModule'}])] + }) + export class MainModule {} + `, + 'src/lazy/lazy.ts': ` + import {NgModule} from '@angular/core'; + + @NgModule() + export class ChildModule {} + `, + }); + const {program, options} = createProgram(['src/main.ts', 'src/lazy/lazy.ts']); + expectNoDiagnosticsInProgram(options, program); + program.listLazyRoutes(); + program.emit(); + + const lazyNgFactory = + fs.readFileSync(path.resolve(testSupport.basePath, 'built/src/lazy/lazy.ngfactory.js')); + expect(lazyNgFactory).toContain('import * as i1 from "./lazy";'); + }); + it('should list lazyRoutes given an entryRoute recursively', () => { writeSomeRoutes(); const {program, options} = createProgram(['src/main.ts']); diff --git a/packages/compiler/src/aot/static_reflector.ts b/packages/compiler/src/aot/static_reflector.ts index 1dd5603166c17..daf6b8ac0d3f4 100644 --- a/packages/compiler/src/aot/static_reflector.ts +++ b/packages/compiler/src/aot/static_reflector.ts @@ -80,8 +80,10 @@ export class StaticReflector implements CompileReflector { const refSymbol = this.symbolResolver.getSymbolByModule(ref.moduleName !, ref.name !, containingFile); const declarationSymbol = this.findSymbolDeclaration(refSymbol); - this.symbolResolver.recordModuleNameForFileName(refSymbol.filePath, ref.moduleName !); - this.symbolResolver.recordImportAs(declarationSymbol, refSymbol); + if (!containingFile) { + this.symbolResolver.recordModuleNameForFileName(refSymbol.filePath, ref.moduleName !); + this.symbolResolver.recordImportAs(declarationSymbol, refSymbol); + } return declarationSymbol; } diff --git a/packages/compiler/test/aot/static_reflector_spec.ts b/packages/compiler/test/aot/static_reflector_spec.ts index f4f90ea1747d7..5403fb8b89ae3 100644 --- a/packages/compiler/test/aot/static_reflector_spec.ts +++ b/packages/compiler/test/aot/static_reflector_spec.ts @@ -1062,6 +1062,22 @@ describe('StaticReflector', () => { .useValue) .toEqual({path: 'foo', data: {e: 1}}); }); + + describe('resolveExternalReference', () => { + it('should register modules names in the StaticSymbolResolver if no containingFile is given', + () => { + init({ + '/tmp/root.ts': ``, + '/tmp/a.ts': `export const x = 1;`, + }); + let symbol = + reflector.resolveExternalReference({moduleName: './a', name: 'x'}, '/tmp/root.ts'); + expect(symbolResolver.getKnownModuleName(symbol.filePath)).toBeFalsy(); + + symbol = reflector.resolveExternalReference({moduleName: 'a', name: 'x'}); + expect(symbolResolver.getKnownModuleName(symbol.filePath)).toBe('a'); + }); + }); }); const DEFAULT_TEST_DATA: {[key: string]: any} = { From ae24430c3c6ea4b27dd8b89ba1e12974475f3a66 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Thu, 26 Oct 2017 09:45:01 -0700 Subject: [PATCH 2/4] fix(compiler): translate emit diagnostics with `noEmitOnError: true`. This prevents errors reported against `.ngfactory.ts` files show up as the result of running `ngc`. Closes #19935 --- .../src/diagnostics/translate_diagnostics.ts | 4 +-- .../compiler-cli/src/transformers/program.ts | 12 ++++--- .../compiler-cli/src/transformers/util.ts | 26 +++++++++++++++ packages/compiler-cli/test/ngc_spec.ts | 32 +++++++++++++++++++ .../test/transformers/program_spec.ts | 31 ++++++++++++++++++ packages/compiler/src/aot/compiler.ts | 18 +++++++---- .../src/view_compiler/type_check_compiler.ts | 13 ++++---- 7 files changed, 116 insertions(+), 20 deletions(-) diff --git a/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts b/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts index 7f5b867bdb28f..aff0162d8bfd6 100644 --- a/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts +++ b/packages/compiler-cli/src/diagnostics/translate_diagnostics.ts @@ -38,10 +38,10 @@ export function translateDiagnostics(host: TypeCheckHost, untranslatedDiagnostic source: SOURCE, code: DEFAULT_ERROR_CODE }); - return; } + } else { + ts.push(diagnostic); } - ts.push(diagnostic); }); return {ts, ng}; } diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index 3ef71c10b734a..ac9a9bb1ef8b8 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/packages/compiler-cli/src/transformers/program.ts @@ -18,7 +18,7 @@ import {CompilerHost, CompilerOptions, CustomTransformers, DEFAULT_ERROR_CODE, D import {CodeGenerator, TsCompilerAotCompilerTypeCheckHostAdapter, getOriginalReferences} from './compiler_host'; import {LowerMetadataCache, getExpressionLoweringTransformFactory} from './lower_expressions'; import {getAngularEmitterTransformFactory} from './node_emitter_transform'; -import {GENERATED_FILES, StructureIsReused, createMessageDiagnostic, isInRootDir, tsStructureIsReused} from './util'; +import {GENERATED_FILES, StructureIsReused, createMessageDiagnostic, isInRootDir, ngToTsDiagnostic, tsStructureIsReused} from './util'; @@ -149,11 +149,9 @@ class AngularCompilerProgram implements Program { getTsSemanticDiagnostics(sourceFile?: ts.SourceFile, cancellationToken?: ts.CancellationToken): ts.Diagnostic[] { - if (sourceFile) { - return this.tsProgram.getSemanticDiagnostics(sourceFile, cancellationToken); - } + const sourceFiles = sourceFile ? [sourceFile] : this.tsProgram.getSourceFiles(); let diags: ts.Diagnostic[] = []; - this.tsProgram.getSourceFiles().forEach(sf => { + sourceFiles.forEach(sf => { if (!GENERATED_FILES.test(sf.fileName)) { diags.push(...this.tsProgram.getSemanticDiagnostics(sf, cancellationToken)); } @@ -300,6 +298,10 @@ class AngularCompilerProgram implements Program { } } this.emittedSourceFiles = emittedSourceFiles; + // translate the diagnostics in the emitResult as well. + const translatedEmitDiags = translateDiagnostics(this.hostAdapter, emitResult.diagnostics); + emitResult.diagnostics = translatedEmitDiags.ts.concat( + this.structuralDiagnostics.concat(translatedEmitDiags.ng).map(ngToTsDiagnostic)); if (!outSrcMapping.length) { // if no files were emitted by TypeScript, also don't emit .json files diff --git a/packages/compiler-cli/src/transformers/util.ts b/packages/compiler-cli/src/transformers/util.ts index 07d7fb80134df..7becf7547abef 100644 --- a/packages/compiler-cli/src/transformers/util.ts +++ b/packages/compiler-cli/src/transformers/util.ts @@ -51,3 +51,29 @@ function pathStartsWithPrefix(prefix: string, fullPath: string): string|null { const rel = path.relative(prefix, fullPath); return rel.startsWith('..') ? null : rel; } + +/** + * Converts a ng.Diagnostic into a ts.Diagnostic. + * This looses some information, and also uses an incomplete object as `file`. + * + * I.e. only use this where the API allows only a ts.Diagnostic. + */ +export function ngToTsDiagnostic(ng: Diagnostic): ts.Diagnostic { + let file: ts.SourceFile|undefined; + let start: number|undefined; + let length: number|undefined; + if (ng.span) { + // Note: We can't use a real ts.SourceFile, + // but we can at least mirror the properties `fileName` and `text`, which + // are mostly used for error reporting. + file = { fileName: ng.span.start.file.url, text: ng.span.start.file.content } as ts.SourceFile; + start = ng.span.start.offset; + length = ng.span.end.offset - start; + } + return { + file, + messageText: ng.messageText, + category: ng.category, + code: ng.code, start, length, + }; +} diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index d51b49fdc49e1..bf90700146609 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -1467,5 +1467,37 @@ describe('ngc transformer command-line', () => { expect(exitCode).toBe(2, 'Compile was expected to fail'); expect(messages[0]).toContain(['Tagged template expressions are not supported in metadata']); }); + + it('should allow using 2 classes with the same name in declarations with noEmitOnError=true', + () => { + write('src/tsconfig.json', `{ + "extends": "../tsconfig-base.json", + "compilerOptions": { + "noEmitOnError": true + }, + "files": ["test-module.ts"] + }`); + function writeComp(fileName: string) { + write(fileName, ` + import {Component} from '@angular/core'; + + @Component({selector: 'comp', template: ''}) + export class TestComponent {} + `); + } + writeComp('src/comp1.ts'); + writeComp('src/comp2.ts'); + write('src/test-module.ts', ` + import {NgModule} from '@angular/core'; + import {TestComponent as Comp1} from './comp1'; + import {TestComponent as Comp2} from './comp2'; + + @NgModule({ + declarations: [Comp1, Comp2], + }) + export class MyModule {} + `); + expect(main(['-p', path.join(basePath, 'src/tsconfig.json')])).toBe(0); + }); }); }); diff --git a/packages/compiler-cli/test/transformers/program_spec.ts b/packages/compiler-cli/test/transformers/program_spec.ts index 19c945d101cdd..8bc67cfbf5a9f 100644 --- a/packages/compiler-cli/test/transformers/program_spec.ts +++ b/packages/compiler-cli/test/transformers/program_spec.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as ts from 'typescript'; +import {formatDiagnostics} from '../../src/perform_compile'; import {CompilerHost, EmitFlags, LazyRoute} from '../../src/transformers/api'; import {createSrcToOutPathMapper} from '../../src/transformers/program'; import {GENERATED_FILES, StructureIsReused, tsStructureIsReused} from '../../src/transformers/util'; @@ -858,4 +859,34 @@ describe('ng program', () => { }]); }); }); + + it('should report errors for ts and ng errors on emit with noEmitOnError=true', () => { + testSupport.writeFiles({ + 'src/main.ts': ` + import {Component, NgModule} from '@angular/core'; + + // Ts error + let x: string = 1; + + // Ng error + @Component({selector: 'comp', templateUrl: './main.html'}) + export class MyComp {} + + @NgModule({declarations: [MyComp]}) + export class MyModule {} + `, + 'src/main.html': '{{nonExistent}}' + }); + const options = testSupport.createCompilerOptions({noEmitOnError: true}); + const host = ng.createCompilerHost({options}); + const program1 = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/main.ts')], options, host}); + const errorDiags = + program1.emit().diagnostics.filter(d => d.category === ts.DiagnosticCategory.Error); + expect(formatDiagnostics(errorDiags)) + .toContain(`src/main.ts(5,13): error TS2322: Type '1' is not assignable to type 'string'.`); + expect(formatDiagnostics(errorDiags)) + .toContain( + `src/main.html(1,1): error TS100: Property 'nonExistent' does not exist on type 'MyComp'.`); + }); }); diff --git a/packages/compiler/src/aot/compiler.ts b/packages/compiler/src/aot/compiler.ts index 3e2eda7ef56ae..22be0661ae092 100644 --- a/packages/compiler/src/aot/compiler.ts +++ b/packages/compiler/src/aot/compiler.ts @@ -193,6 +193,7 @@ export class AotCompiler { private _createNgFactoryStub( outputCtx: OutputContext, file: NgAnalyzedFile, emitFlags: StubEmitFlags) { + let componentId = 0; file.ngModules.forEach((ngModuleMeta, ngModuleIndex) => { // Note: the code below needs to executed for StubEmitFlags.Basic and StubEmitFlags.TypeCheck, // so we don't change the .ngfactory file too much when adding the typecheck block. @@ -230,12 +231,14 @@ export class AotCompiler { if (!compMeta.isComponent) { return; } + componentId++; this._createTypeCheckBlock( - outputCtx, ngModuleMeta, this._metadataResolver.getHostComponentMetadata(compMeta), - [compMeta.type], externalReferenceVars); - this._createTypeCheckBlock( - outputCtx, ngModuleMeta, compMeta, ngModuleMeta.transitiveModule.directives, + outputCtx, `${compMeta.type.reference.name}_Host_${componentId}`, ngModuleMeta, + this._metadataResolver.getHostComponentMetadata(compMeta), [compMeta.type], externalReferenceVars); + this._createTypeCheckBlock( + outputCtx, `${compMeta.type.reference.name}_${componentId}`, ngModuleMeta, compMeta, + ngModuleMeta.transitiveModule.directives, externalReferenceVars); }); } }); @@ -246,12 +249,13 @@ export class AotCompiler { } private _createTypeCheckBlock( - ctx: OutputContext, moduleMeta: CompileNgModuleMetadata, compMeta: CompileDirectiveMetadata, - directives: CompileIdentifierMetadata[], externalReferenceVars: Map) { + ctx: OutputContext, componentId: string, moduleMeta: CompileNgModuleMetadata, + compMeta: CompileDirectiveMetadata, directives: CompileIdentifierMetadata[], + externalReferenceVars: Map) { const {template: parsedTemplate, pipes: usedPipes} = this._parseTemplate(compMeta, moduleMeta, directives); ctx.statements.push(...this._typeCheckCompiler.compileComponent( - compMeta, parsedTemplate, usedPipes, externalReferenceVars)); + componentId, compMeta, parsedTemplate, usedPipes, externalReferenceVars)); } emitMessageBundle(analyzeResult: NgAnalyzedModules, locale: string|null): MessageBundle { diff --git a/packages/compiler/src/view_compiler/type_check_compiler.ts b/packages/compiler/src/view_compiler/type_check_compiler.ts index 9e5f58c6f36dd..3b4597979e312 100644 --- a/packages/compiler/src/view_compiler/type_check_compiler.ts +++ b/packages/compiler/src/view_compiler/type_check_compiler.ts @@ -9,7 +9,7 @@ import {AotCompilerOptions} from '../aot/compiler_options'; import {StaticReflector} from '../aot/static_reflector'; import {StaticSymbol} from '../aot/static_symbol'; -import {CompileDiDependencyMetadata, CompileDirectiveMetadata, CompilePipeSummary, viewClassName} from '../compile_metadata'; +import {CompileDiDependencyMetadata, CompileDirectiveMetadata, CompilePipeSummary} from '../compile_metadata'; import {BuiltinConverter, EventHandlerVars, LocalResolver, convertActionBinding, convertPropertyBinding, convertPropertyBindingBuiltins} from '../compiler_util/expression_converter'; import {AST, ASTWithSource, Interpolation} from '../expression_parser/ast'; import {Identifiers} from '../identifiers'; @@ -33,7 +33,8 @@ export class TypeCheckCompiler { * and also violate the point above. */ compileComponent( - component: CompileDirectiveMetadata, template: TemplateAst[], usedPipes: CompilePipeSummary[], + componentId: string, component: CompileDirectiveMetadata, template: TemplateAst[], + usedPipes: CompilePipeSummary[], externalReferenceVars: Map): o.Statement[] { const pipes = new Map(); usedPipes.forEach(p => pipes.set(p.name, p.type.reference)); @@ -48,7 +49,7 @@ export class TypeCheckCompiler { const visitor = viewBuilderFactory(null); visitor.visitAll([], template); - return visitor.build(); + return visitor.build(componentId); } } @@ -103,8 +104,8 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { templateVisitAll(this, astNodes); } - build(targetStatements: o.Statement[] = []): o.Statement[] { - this.children.forEach((child) => child.build(targetStatements)); + build(componentId: string, targetStatements: o.Statement[] = []): o.Statement[] { + this.children.forEach((child) => child.build(componentId, targetStatements)); const viewStmts: o.Statement[] = [o.variable(DYNAMIC_VAR_NAME).set(o.NULL_EXPR).toDeclStmt(o.DYNAMIC_TYPE)]; let bindingCount = 0; @@ -128,7 +129,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { (stmt: o.Statement) => o.applySourceSpanToStatementIfNeeded(stmt, sourceSpan))); }); - const viewName = `_View_${this.component.name}_${this.embeddedViewIndex}`; + const viewName = `_View_${componentId}_${this.embeddedViewIndex}`; const viewFactory = new o.DeclareFunctionStmt(viewName, [], viewStmts); targetStatements.push(viewFactory); return targetStatements; From a08da7076e3b2ebfacb5e490a0546ea1aeb99928 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Thu, 26 Oct 2017 15:24:54 -0700 Subject: [PATCH 3/4] fix(compiler): recover from structural errors in watch mode This also changes the compiler so that we throw less often on structural changes and produce a meaningful state in the `ng.Program` in case of errors. Related to #19951 --- packages/compiler-cli/src/perform_watch.ts | 4 ++ .../compiler-cli/src/transformers/program.ts | 69 ++++++++++--------- packages/compiler-cli/test/ngc_spec.ts | 2 +- .../compiler-cli/test/perform_watch_spec.ts | 58 +++++++++++++--- packages/compiler-cli/test/test_support.ts | 4 +- .../test/transformers/program_spec.ts | 62 +++++++++++++++++ packages/compiler/src/aot/static_reflector.ts | 2 +- 7 files changed, 157 insertions(+), 44 deletions(-) diff --git a/packages/compiler-cli/src/perform_watch.ts b/packages/compiler-cli/src/perform_watch.ts index c5acc570f0b23..2943bc7f5c48e 100644 --- a/packages/compiler-cli/src/perform_watch.ts +++ b/packages/compiler-cli/src/perform_watch.ts @@ -191,6 +191,10 @@ export function performWatchCompilation(host: PerformWatchHost): }; } ingoreFilesForWatch.clear(); + const oldProgram = cachedProgram; + // We clear out the `cachedProgram` here as a + // program can only be used as `oldProgram` 1x + cachedProgram = undefined; const compileResult = performCompilation({ rootNames: cachedOptions.rootNames, options: cachedOptions.options, diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index ac9a9bb1ef8b8..27bf90cf7c712 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/packages/compiler-cli/src/transformers/program.ts @@ -175,15 +175,17 @@ class AngularCompilerProgram implements Program { if (this._analyzedModules) { throw new Error('Angular structure already loaded'); } - const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); - return this.compiler.loadFilesAsync(sourceFiles) - .catch(this.catchAnalysisError.bind(this)) - .then(analyzedModules => { - if (this._analyzedModules) { - throw new Error('Angular structure loaded both synchronously and asynchronsly'); - } - this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); - }); + return Promise.resolve() + .then(() => { + const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); + return this.compiler.loadFilesAsync(sourceFiles).then(analyzedModules => { + if (this._analyzedModules) { + throw new Error('Angular structure loaded both synchronously and asynchronsly'); + } + this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); + }); + }) + .catch(e => this._createProgramOnError(e)); } listLazyRoutes(route?: string): LazyRoute[] { @@ -402,14 +404,13 @@ class AngularCompilerProgram implements Program { if (this._analyzedModules) { return; } - const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); - let analyzedModules: NgAnalyzedModules|null; try { - analyzedModules = this.compiler.loadFilesSync(sourceFiles); + const {tmpProgram, sourceFiles, rootNames} = this._createProgramWithBasicStubs(); + const analyzedModules = this.compiler.loadFilesSync(sourceFiles); + this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); } catch (e) { - analyzedModules = this.catchAnalysisError(e); + this._createProgramOnError(e); } - this._updateProgramWithTypeCheckStubs(tmpProgram, analyzedModules, rootNames); } private _createCompiler() { @@ -457,7 +458,7 @@ class AngularCompilerProgram implements Program { let rootNames = this.rootNames; if (this.options.generateCodeForLibraries !== false) { - // if we should generateCodeForLibraries, enver include + // if we should generateCodeForLibraries, never include // generated files in the program as otherwise we will // ovewrite them and typescript will report the error // TS5055: Cannot write file ... because it would overwrite input file. @@ -482,23 +483,21 @@ class AngularCompilerProgram implements Program { } private _updateProgramWithTypeCheckStubs( - tmpProgram: ts.Program, analyzedModules: NgAnalyzedModules|null, rootNames: string[]) { - this._analyzedModules = analyzedModules || emptyModules; - if (analyzedModules) { - tmpProgram.getSourceFiles().forEach(sf => { - if (sf.fileName.endsWith('.ngfactory.ts')) { - const {generate, baseFileName} = this.hostAdapter.shouldGenerateFile(sf.fileName); - if (generate) { - // Note: ! is ok as hostAdapter.shouldGenerateFile will always return a basefileName - // for .ngfactory.ts files. - const genFile = this.compiler.emitTypeCheckStub(sf.fileName, baseFileName !); - if (genFile) { - this.hostAdapter.updateGeneratedFile(genFile); - } + tmpProgram: ts.Program, analyzedModules: NgAnalyzedModules, rootNames: string[]) { + this._analyzedModules = analyzedModules; + tmpProgram.getSourceFiles().forEach(sf => { + if (sf.fileName.endsWith('.ngfactory.ts')) { + const {generate, baseFileName} = this.hostAdapter.shouldGenerateFile(sf.fileName); + if (generate) { + // Note: ! is ok as hostAdapter.shouldGenerateFile will always return a basefileName + // for .ngfactory.ts files. + const genFile = this.compiler.emitTypeCheckStub(sf.fileName, baseFileName !); + if (genFile) { + this.hostAdapter.updateGeneratedFile(genFile); } } - }); - } + } + }); this._tsProgram = ts.createProgram(rootNames, this.options, this.hostAdapter, tmpProgram); // Note: the new ts program should be completely reusable by TypeScript as: // - we cache all the files in the hostAdapter @@ -509,7 +508,13 @@ class AngularCompilerProgram implements Program { } } - private catchAnalysisError(e: any): NgAnalyzedModules|null { + private _createProgramOnError(e: any) { + // Still fill the analyzedModules and the tsProgram + // so that we don't cause other errors for users who e.g. want to emit the ngProgram. + this._analyzedModules = emptyModules; + this.oldTsProgram = undefined; + this._hostAdapter.isSourceFile = () => false; + this._tsProgram = ts.createProgram(this.rootNames, this.options, this.hostAdapter); if (isSyntaxError(e)) { const parserErrors = getParseErrors(e); if (parserErrors && parserErrors.length) { @@ -533,7 +538,7 @@ class AngularCompilerProgram implements Program { } ]; } - return null; + return; } throw e; } diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index bf90700146609..58911fcd67f1a 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -1464,7 +1464,7 @@ describe('ngc transformer command-line', () => { const messages: string[] = []; const exitCode = main(['-p', path.join(basePath, 'src/tsconfig.json')], message => messages.push(message)); - expect(exitCode).toBe(2, 'Compile was expected to fail'); + expect(exitCode).toBe(1, 'Compile was expected to fail'); expect(messages[0]).toContain(['Tagged template expressions are not supported in metadata']); }); diff --git a/packages/compiler-cli/test/perform_watch_spec.ts b/packages/compiler-cli/test/perform_watch_spec.ts index ae0184e2c296f..4cd19b7c6a9e7 100644 --- a/packages/compiler-cli/test/perform_watch_spec.ts +++ b/packages/compiler-cli/test/perform_watch_spec.ts @@ -105,6 +105,47 @@ describe('perform watch', () => { expect(getSourceFileSpy !).toHaveBeenCalledWith(mainTsPath, ts.ScriptTarget.ES5); expect(getSourceFileSpy !).toHaveBeenCalledWith(utilTsPath, ts.ScriptTarget.ES5); }); + + it('should recover from static analysis errors', () => { + const config = createConfig(); + const host = new MockWatchHost(config); + + const okFileContent = ` + import {NgModule} from '@angular/core'; + + @NgModule() + export class MyModule {} + `; + const errorFileContent = ` + import {NgModule} from '@angular/core'; + + @NgModule(() => (1===1 ? null as any : null as any)) + export class MyModule {} + `; + const indexTsPath = path.resolve(testSupport.basePath, 'src', 'index.ts'); + + testSupport.write(indexTsPath, okFileContent); + + performWatchCompilation(host); + expectNoDiagnostics(config.options, host.diagnostics); + + // Do it multiple times as the watch mode switches internal modes. + // E.g. from regular compile to using summaries, ... + for (let i = 0; i < 3; i++) { + host.diagnostics = []; + testSupport.write(indexTsPath, okFileContent); + host.triggerFileChange(FileChangeEvent.Change, indexTsPath); + expectNoDiagnostics(config.options, host.diagnostics); + + host.diagnostics = []; + testSupport.write(indexTsPath, errorFileContent); + host.triggerFileChange(FileChangeEvent.Change, indexTsPath); + + const errDiags = host.diagnostics.filter(d => d.category === ts.DiagnosticCategory.Error); + expect(errDiags.length).toBe(1); + expect(errDiags[0].messageText).toContain('Function calls are not supported.'); + } + }); }); function createModuleAndCompSource(prefix: string, template: string = prefix + 'template') { @@ -122,7 +163,8 @@ function createModuleAndCompSource(prefix: string, template: string = prefix + ' } class MockWatchHost { - timeoutListeners: Array<(() => void)|null> = []; + nextTimeoutListenerId = 1; + timeoutListeners: {[id: string]: (() => void)} = {}; fileChangeListeners: Array<((event: FileChangeEvent, fileName: string) => void)|null> = []; diagnostics: ng.Diagnostics = []; constructor(public config: ng.ParsedConfiguration) {} @@ -141,16 +183,16 @@ class MockWatchHost { close: () => this.fileChangeListeners[id] = null, }; } - setTimeout(callback: () => void, ms: number): any { - const id = this.timeoutListeners.length; - this.timeoutListeners.push(callback); + setTimeout(callback: () => void): any { + const id = this.nextTimeoutListenerId++; + this.timeoutListeners[id] = callback; return id; } - clearTimeout(timeoutId: any): void { this.timeoutListeners[timeoutId] = null; } + clearTimeout(timeoutId: any): void { delete this.timeoutListeners[timeoutId]; } flushTimeouts() { - this.timeoutListeners.forEach(cb => { - if (cb) cb(); - }); + const listeners = this.timeoutListeners; + this.timeoutListeners = {}; + Object.keys(listeners).forEach(id => listeners[id]()); } triggerFileChange(event: FileChangeEvent, fileName: string) { this.fileChangeListeners.forEach(listener => { diff --git a/packages/compiler-cli/test/test_support.ts b/packages/compiler-cli/test/test_support.ts index 122bb644929c5..c64be8b91f8b7 100644 --- a/packages/compiler-cli/test/test_support.ts +++ b/packages/compiler-cli/test/test_support.ts @@ -64,10 +64,10 @@ export function setup(): TestSupport { function write(fileName: string, content: string) { const dir = path.dirname(fileName); if (dir != '.') { - const newDir = path.join(basePath, dir); + const newDir = path.resolve(basePath, dir); if (!fs.existsSync(newDir)) fs.mkdirSync(newDir); } - fs.writeFileSync(path.join(basePath, fileName), content, {encoding: 'utf-8'}); + fs.writeFileSync(path.resolve(basePath, fileName), content, {encoding: 'utf-8'}); } function writeFiles(...mockDirs: {[fileName: string]: string}[]) { diff --git a/packages/compiler-cli/test/transformers/program_spec.ts b/packages/compiler-cli/test/transformers/program_spec.ts index 8bc67cfbf5a9f..ecb379fa21678 100644 --- a/packages/compiler-cli/test/transformers/program_spec.ts +++ b/packages/compiler-cli/test/transformers/program_spec.ts @@ -889,4 +889,66 @@ describe('ng program', () => { .toContain( `src/main.html(1,1): error TS100: Property 'nonExistent' does not exist on type 'MyComp'.`); }); + + describe('errors', () => { + const fileWithStructuralError = ` + import {NgModule} from '@angular/core'; + + @NgModule(() => (1===1 ? null as any : null as any)) + export class MyModule {} + `; + const fileWithGoodContent = ` + import {NgModule} from '@angular/core'; + + @NgModule() + export class MyModule {} + `; + + it('should not throw on structural errors but collect them', () => { + testSupport.write('src/index.ts', fileWithStructuralError); + + const options = testSupport.createCompilerOptions(); + const host = ng.createCompilerHost({options}); + const program = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], options, host}); + + const structuralErrors = program.getNgStructuralDiagnostics(); + expect(structuralErrors.length).toBe(1); + expect(structuralErrors[0].messageText).toContain('Function calls are not supported.'); + }); + + it('should not throw on structural errors but collect them (loadNgStructureAsync)', (done) => { + testSupport.write('src/index.ts', fileWithStructuralError); + + const options = testSupport.createCompilerOptions(); + const host = ng.createCompilerHost({options}); + const program = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], options, host}); + program.loadNgStructureAsync().then(() => { + const structuralErrors = program.getNgStructuralDiagnostics(); + expect(structuralErrors.length).toBe(1); + expect(structuralErrors[0].messageText).toContain('Function calls are not supported.'); + done(); + }); + }); + + it('should be able to use a program with structural errors as oldProgram', () => { + testSupport.write('src/index.ts', fileWithStructuralError); + + const options = testSupport.createCompilerOptions(); + const host = ng.createCompilerHost({options}); + const program1 = ng.createProgram( + {rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], options, host}); + expect(program1.getNgStructuralDiagnostics().length).toBe(1); + + testSupport.write('src/index.ts', fileWithGoodContent); + const program2 = ng.createProgram({ + rootNames: [path.resolve(testSupport.basePath, 'src/index.ts')], + options, + host, + oldProgram: program1 + }); + expectNoDiagnosticsInProgram(options, program2); + }); + }); }); diff --git a/packages/compiler/src/aot/static_reflector.ts b/packages/compiler/src/aot/static_reflector.ts index daf6b8ac0d3f4..5300134cc15a4 100644 --- a/packages/compiler/src/aot/static_reflector.ts +++ b/packages/compiler/src/aot/static_reflector.ts @@ -752,7 +752,7 @@ class PopulatedScope extends BindingScope { } function positionalError(message: string, fileName: string, line: number, column: number): Error { - const result = new Error(message); + const result = syntaxError(message); (result as any).fileName = fileName; (result as any).line = line; (result as any).column = column; From 67b2ce65bf0b62c367960ee64e8f652988db865a Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Thu, 26 Oct 2017 15:27:21 -0700 Subject: [PATCH 4/4] fix(compiler): make watch mode work on windows Fixes #19951 --- packages/compiler-cli/src/perform_watch.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/compiler-cli/src/perform_watch.ts b/packages/compiler-cli/src/perform_watch.ts index 2943bc7f5c48e..d27026eed71c3 100644 --- a/packages/compiler-cli/src/perform_watch.ts +++ b/packages/compiler-cli/src/perform_watch.ts @@ -129,6 +129,7 @@ export function performWatchCompilation(host: PerformWatchHost): return {close, ready: cb => readyPromise.then(cb), firstCompileResult}; function cacheEntry(fileName: string): CacheEntry { + fileName = path.normalize(fileName); let entry = fileCache.get(fileName); if (!entry) { entry = {}; @@ -249,7 +250,7 @@ export function performWatchCompilation(host: PerformWatchHost): if (event === FileChangeEvent.CreateDeleteDir) { fileCache.clear(); } else { - fileCache.delete(fileName); + fileCache.delete(path.normalize(fileName)); } if (!ingoreFilesForWatch.has(path.normalize(fileName))) {