Skip to content

Commit

Permalink
fix(compiler): switch to modern diagnostic formatting (#34234)
Browse files Browse the repository at this point in the history
The compiler exports a `formatDiagnostics` function which consumers can use
to print both ts and ng diagnostics. However, this function was previously
using the "old" style TypeScript diagnostics, as opposed to the modern
diagnostic printer which uses terminal colors and prints additional context
information.

This commit updates `formatDiagnostics` to use the modern formatter, plus to
update Ivy's negative error codes to Angular 'NG' errors.

The Angular CLI needs a little more work to use this function for printing
TS diagnostics, but this commit alone should fix Bazel builds as ngc-wrapped
goes through `formatDiagnostics`.

PR Close #34234
  • Loading branch information
alxhub authored and AndrewKushnir committed Dec 9, 2019
1 parent e3d829f commit 60051eb
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 28 deletions.
11 changes: 1 addition & 10 deletions packages/compiler-cli/src/main.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -240,17 +240,8 @@ function printDiagnostics(
if (diagnostics.length === 0) { if (diagnostics.length === 0) {
return; return;
} }

const formatHost = getFormatDiagnosticsHost(options); const formatHost = getFormatDiagnosticsHost(options);
if (options && options.enableIvy !== false) { consoleError(formatDiagnostics(diagnostics, formatHost));
const ngDiagnostics = diagnostics.filter(api.isNgDiagnostic);
const tsDiagnostics = diagnostics.filter(api.isTsDiagnostic);
consoleError(replaceTsWithNgInErrors(
ts.formatDiagnosticsWithColorAndContext(tsDiagnostics, formatHost)));
consoleError(formatDiagnostics(ngDiagnostics, formatHost));
} else {
consoleError(formatDiagnostics(diagnostics, formatHost));
}
} }


// CLI entry point // CLI entry point
Expand Down
6 changes: 5 additions & 1 deletion packages/compiler-cli/src/perform_compile.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@


import {Position, isSyntaxError} from '@angular/compiler'; import {Position, isSyntaxError} from '@angular/compiler';
import * as ts from 'typescript'; import * as ts from 'typescript';

import {AbsoluteFsPath, absoluteFrom, getFileSystem, relative, resolve} from '../src/ngtsc/file_system'; import {AbsoluteFsPath, absoluteFrom, getFileSystem, relative, resolve} from '../src/ngtsc/file_system';

import {replaceTsWithNgInErrors} from './ngtsc/diagnostics';
import * as api from './transformers/api'; import * as api from './transformers/api';
import * as ng from './transformers/entry_points'; import * as ng from './transformers/entry_points';
import {createMessageDiagnostic} from './transformers/util'; import {createMessageDiagnostic} from './transformers/util';
Expand Down Expand Up @@ -94,7 +97,8 @@ export function formatDiagnostics(
return diags return diags
.map(diagnostic => { .map(diagnostic => {
if (api.isTsDiagnostic(diagnostic)) { if (api.isTsDiagnostic(diagnostic)) {
return ts.formatDiagnostics([diagnostic], host); return replaceTsWithNgInErrors(
ts.formatDiagnosticsWithColorAndContext([diagnostic], host));
} else { } else {
return formatDiagnostic(diagnostic, host); return formatDiagnostic(diagnostic, host);
} }
Expand Down
30 changes: 18 additions & 12 deletions packages/compiler-cli/test/ngc_spec.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import * as path from 'path';
import * as ts from 'typescript'; import * as ts from 'typescript';


import {main, readCommandLineAndConfiguration, watchMode} from '../src/main'; import {main, readCommandLineAndConfiguration, watchMode} from '../src/main';
import {setup} from './test_support'; import {setup, stripAnsi} from './test_support';


describe('ngc transformer command-line', () => { describe('ngc transformer command-line', () => {
let basePath: string; let basePath: string;
Expand Down Expand Up @@ -89,8 +89,9 @@ describe('ngc transformer command-line', () => {
errorSpy.and.stub(); errorSpy.and.stub();


const exitCode = main(['-p', basePath], errorSpy); const exitCode = main(['-p', basePath], errorSpy);
expect(errorSpy).toHaveBeenCalledWith( const errorText = stripAnsi(errorSpy.calls.mostRecent().args[0]);
`test.ts(1,1): error TS1128: Declaration or statement expected.\r\n`); expect(errorText).toContain(
`test.ts:1:1 - error TS1128: Declaration or statement expected.\r\n`);
expect(exitCode).toBe(1); expect(exitCode).toBe(1);
}); });


Expand All @@ -105,7 +106,8 @@ describe('ngc transformer command-line', () => {
}`); }`);


const exitCode = main(['-p', basePath], errorSpy); const exitCode = main(['-p', basePath], errorSpy);
expect(errorSpy).toHaveBeenCalledWith( const errorText = stripAnsi(errorSpy.calls.mostRecent().args[0]);
expect(errorText).toContain(
`error TS6053: File '` + path.posix.join(basePath, 'test.ts') + `' not found.` + `error TS6053: File '` + path.posix.join(basePath, 'test.ts') + `' not found.` +
'\n'); '\n');
expect(exitCode).toEqual(1); expect(exitCode).toEqual(1);
Expand All @@ -116,8 +118,9 @@ describe('ngc transformer command-line', () => {
write('test.ts', 'foo;'); write('test.ts', 'foo;');


const exitCode = main(['-p', basePath], errorSpy); const exitCode = main(['-p', basePath], errorSpy);
expect(errorSpy).toHaveBeenCalledWith( const errorText = stripAnsi(errorSpy.calls.mostRecent().args[0]);
`test.ts(1,1): error TS2304: Cannot find name 'foo'.` + expect(errorText).toContain(
`test.ts:1:1 - error TS2304: Cannot find name 'foo'.` +
'\n'); '\n');
expect(exitCode).toEqual(1); expect(exitCode).toEqual(1);
}); });
Expand All @@ -127,8 +130,9 @@ describe('ngc transformer command-line', () => {
write('test.ts', `import {MyClass} from './not-exist-deps';`); write('test.ts', `import {MyClass} from './not-exist-deps';`);


const exitCode = main(['-p', basePath], errorSpy); const exitCode = main(['-p', basePath], errorSpy);
expect(errorSpy).toHaveBeenCalledWith( const errorText = stripAnsi(errorSpy.calls.mostRecent().args[0]);
`test.ts(1,23): error TS2307: Cannot find module './not-exist-deps'.` + expect(errorText).toContain(
`test.ts:1:23 - error TS2307: Cannot find module './not-exist-deps'.` +
'\n'); '\n');
expect(exitCode).toEqual(1); expect(exitCode).toEqual(1);
}); });
Expand All @@ -139,8 +143,9 @@ describe('ngc transformer command-line', () => {
write('test.ts', `import {MyClass} from './empty-deps';`); write('test.ts', `import {MyClass} from './empty-deps';`);


const exitCode = main(['-p', basePath], errorSpy); const exitCode = main(['-p', basePath], errorSpy);
expect(errorSpy).toHaveBeenCalledWith( const errorText = stripAnsi(errorSpy.calls.mostRecent().args[0]);
`test.ts(1,9): error TS2305: Module '"./empty-deps"' has no exported member 'MyClass'.\n`); expect(errorText).toContain(
`test.ts:1:9 - error TS2305: Module '"./empty-deps"' has no exported member 'MyClass'.\n`);
expect(exitCode).toEqual(1); expect(exitCode).toEqual(1);
}); });


Expand All @@ -153,8 +158,9 @@ describe('ngc transformer command-line', () => {
`); `);


const exitCode = main(['-p', basePath], errorSpy); const exitCode = main(['-p', basePath], errorSpy);
expect(errorSpy).toHaveBeenCalledWith( const errorText = stripAnsi(errorSpy.calls.mostRecent().args[0]);
'test.ts(3,9): error TS2349: This expression is not callable.\n' + expect(errorText).toContain(
'test.ts:3:9 - error TS2349: This expression is not callable.\n' +
' Type \'String\' has no call signatures.\n'); ' Type \'String\' has no call signatures.\n');
expect(exitCode).toEqual(1); expect(exitCode).toEqual(1);
}); });
Expand Down
6 changes: 6 additions & 0 deletions packages/compiler-cli/test/test_support.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -171,3 +171,9 @@ export function expectNoDiagnosticsInProgram(options: ng.CompilerOptions, p: ng.
export function normalizeSeparators(path: string): string { export function normalizeSeparators(path: string): string {
return path.replace(/\\/g, '/'); return path.replace(/\\/g, '/');
} }

const STRIP_ANSI = /\x1B\x5B\d+m/g;

export function stripAnsi(diags: string): string {
return diags.replace(STRIP_ANSI, '');
}
10 changes: 5 additions & 5 deletions packages/compiler-cli/test/transformers/program_spec.ts
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {formatDiagnostics} from '../../src/perform_compile';
import {CompilerHost, EmitFlags, LazyRoute} from '../../src/transformers/api'; import {CompilerHost, EmitFlags, LazyRoute} from '../../src/transformers/api';
import {createSrcToOutPathMapper} from '../../src/transformers/program'; import {createSrcToOutPathMapper} from '../../src/transformers/program';
import {StructureIsReused, tsStructureIsReused} from '../../src/transformers/util'; import {StructureIsReused, tsStructureIsReused} from '../../src/transformers/util';
import {TestSupport, expectNoDiagnosticsInProgram, setup} from '../test_support'; import {TestSupport, expectNoDiagnosticsInProgram, setup, stripAnsi} from '../test_support';


describe('ng program', () => { describe('ng program', () => {
let testSupport: TestSupport; let testSupport: TestSupport;
Expand Down Expand Up @@ -986,11 +986,11 @@ describe('ng program', () => {
{rootNames: [path.resolve(testSupport.basePath, 'src/main.ts')], options, host}); {rootNames: [path.resolve(testSupport.basePath, 'src/main.ts')], options, host});
const errorDiags = const errorDiags =
program1.emit().diagnostics.filter(d => d.category === ts.DiagnosticCategory.Error); program1.emit().diagnostics.filter(d => d.category === ts.DiagnosticCategory.Error);
expect(formatDiagnostics(errorDiags)) expect(stripAnsi(formatDiagnostics(errorDiags)))
.toContain(`src/main.ts(5,13): error TS2322: Type '1' is not assignable to type 'string'.`); .toContain(`src/main.ts:5:13 - error TS2322: Type '1' is not assignable to type 'string'.`);
expect(formatDiagnostics(errorDiags)) expect(stripAnsi(formatDiagnostics(errorDiags)))
.toContain( .toContain(
`src/main.html(1,1): error TS100: Property 'nonExistent' does not exist on type 'MyComp'.`); `src/main.html:1:1 - error TS100: Property 'nonExistent' does not exist on type 'MyComp'.`);
}); });


it('should not report emit errors with noEmitOnError=false', () => { it('should not report emit errors with noEmitOnError=false', () => {
Expand Down

0 comments on commit 60051eb

Please sign in to comment.