Skip to content

Commit af63489

Browse files
filipesilvafilipesilva
committed
fix(@ngtools/webpack): fix and improve error reporting
Fix angular#7925
1 parent 1abd7b0 commit af63489

File tree

7 files changed

+385
-202
lines changed

7 files changed

+385
-202
lines changed

‎packages/@ngtools/webpack/src/angular_compiler_plugin.ts

Lines changed: 198 additions & 155 deletions
Large diffs are not rendered by default.

‎packages/@ngtools/webpack/src/gather_diagnostics.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,6 @@ export function gatherDiagnostics(
7272
checkDiagnostics(angularProgram.getTsSemanticDiagnostics(undefined, cancellationToken));
7373
timeEnd(`${benchmarkLabel}.gatherDiagnostics.ng.getTsSemanticDiagnostics`);
7474

75-
// Check Angular structural diagnostics.
76-
time(`${benchmarkLabel}.gatherDiagnostics.ng.getNgStructuralDiagnostics`);
77-
checkOtherDiagnostics = checkOtherDiagnostics &&
78-
checkDiagnostics(angularProgram.getNgStructuralDiagnostics(cancellationToken));
79-
timeEnd(`${benchmarkLabel}.gatherDiagnostics.ng.getNgStructuralDiagnostics`);
80-
8175
// Check Angular semantic diagnostics
8276
time(`${benchmarkLabel}.gatherDiagnostics.ng.getNgSemanticDiagnostics`);
8377
checkOtherDiagnostics = checkOtherDiagnostics &&

‎packages/@ngtools/webpack/src/loader.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,7 @@ export function ngcLoader(this: LoaderContext & { _compilation: any }, source: s
567567
plugin.done
568568
.then(() => {
569569
timeEnd(timeLabel + '.ngcLoader.AngularCompilerPlugin');
570-
const result = plugin.getFile(sourceFileName);
570+
const result = plugin.getCompiledFile(sourceFileName);
571571

572572
if (result.sourceMap) {
573573
// Process sourcemaps for Webpack.
@@ -579,12 +579,9 @@ export function ngcLoader(this: LoaderContext & { _compilation: any }, source: s
579579
result.sourceMap = JSON.stringify(sourceMap);
580580
}
581581

582-
timeEnd(timeLabel);
583-
if (result.outputText === undefined) {
584-
throw new Error('TypeScript compilation failed.');
585-
}
586-
587582
// Dependencies must use system path separator.
583+
// TODO: move the denormalizer into it's own helper.
584+
result.errorDependencies.forEach(dep => this.addDependency(dep.replace(/\//g, path.sep)));
588585
const dependencies = plugin.getDependencies(sourceFileName);
589586
dependencies.forEach(dep => this.addDependency(dep.replace(/\//g, path.sep)));
590587

@@ -596,10 +593,11 @@ export function ngcLoader(this: LoaderContext & { _compilation: any }, source: s
596593
origDependencies.forEach(dep => this.addDependency(dep.replace(/\//g, path.sep)));
597594
}
598595

596+
timeEnd(timeLabel);
599597
cb(null, result.outputText, result.sourceMap);
600598
})
601599
.catch(err => {
602-
timeEnd(timeLabel + '.ngcLoader.AngularCompilerPlugin');
600+
timeEnd(timeLabel);
603601
cb(err);
604602
});
605603
} else if (plugin instanceof AotPlugin) {

‎packages/@ngtools/webpack/src/type_checker.ts

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -51,31 +51,36 @@ let lastCancellationToken: CancellationToken;
5151

5252
process.on('message', (message: TypeCheckerMessage) => {
5353
time('TypeChecker.message');
54-
switch (message.kind) {
55-
case MESSAGE_KIND.Init:
56-
const initMessage = message as InitMessage;
57-
typeChecker = new TypeChecker(
58-
initMessage.compilerOptions,
59-
initMessage.basePath,
60-
initMessage.jitMode,
61-
initMessage.tsFilenames,
62-
);
63-
break;
64-
case MESSAGE_KIND.Update:
65-
if (!typeChecker) {
66-
throw new Error('TypeChecker: update message received before initialization');
67-
}
68-
if (lastCancellationToken) {
69-
// This cancellation token doesn't seem to do much, messages don't seem to be processed
70-
// before the diagnostics finish.
71-
lastCancellationToken.requestCancellation();
72-
}
73-
const updateMessage = message as UpdateMessage;
74-
lastCancellationToken = new CancellationToken();
75-
typeChecker.update(updateMessage.changedTsFiles, lastCancellationToken);
76-
break;
77-
default:
78-
throw new Error(`TypeChecker: Unexpected message received: ${message}.`);
54+
try {
55+
switch (message.kind) {
56+
case MESSAGE_KIND.Init:
57+
const initMessage = message as InitMessage;
58+
typeChecker = new TypeChecker(
59+
initMessage.compilerOptions,
60+
initMessage.basePath,
61+
initMessage.jitMode,
62+
initMessage.tsFilenames,
63+
);
64+
break;
65+
case MESSAGE_KIND.Update:
66+
if (!typeChecker) {
67+
throw new Error('TypeChecker: update message received before initialization');
68+
}
69+
if (lastCancellationToken) {
70+
// This cancellation token doesn't seem to do much, messages don't seem to be processed
71+
// before the diagnostics finish.
72+
lastCancellationToken.requestCancellation();
73+
}
74+
const updateMessage = message as UpdateMessage;
75+
lastCancellationToken = new CancellationToken();
76+
typeChecker.update(updateMessage.changedTsFiles, lastCancellationToken);
77+
break;
78+
default:
79+
throw new Error(`TypeChecker: Unexpected message received: ${message}.`);
80+
}
81+
} catch (error) {
82+
// Ignore errors in the TypeChecker.
83+
// Anything that would throw here will error out the compilation as well.
7984
}
8085
timeEnd('TypeChecker.message');
8186
});

‎tests/e2e/tests/build/build-errors.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { ng } from '../../utils/process';
2+
import { updateJsonFile } from '../../utils/project';
3+
import { writeFile, appendToFile, readFile } from '../../utils/fs';
4+
import { getGlobalVariable } from '../../utils/env';
5+
import { expectToFail } from '../../utils/utils';
6+
7+
8+
const extraErrors = [
9+
`Final loader didn't return a Buffer or String`,
10+
`doesn't contain a valid alias configuration`,
11+
`main.ts is not part of the TypeScript compilation.`,
12+
];
13+
14+
export default function () {
15+
if (process.platform.startsWith('win')) {
16+
return Promise.resolve();
17+
}
18+
// Skip this in ejected tests.
19+
if (getGlobalVariable('argv').eject) {
20+
return Promise.resolve();
21+
}
22+
23+
// Skip in non-nightly tests. Switch this check around when ng5 is out.
24+
if (!getGlobalVariable('argv').nightly) {
25+
return Promise.resolve();
26+
}
27+
28+
let origContent: string;
29+
30+
return Promise.resolve()
31+
// Save the original contents of `./src/app/app.component.ts`.
32+
.then(() => readFile('./src/app/app.component.ts'))
33+
.then((contents) => origContent = contents)
34+
// Check `part of the TypeScript compilation` errors.
35+
// These should show an error only for the missing file.
36+
.then(() => updateJsonFile('./src/tsconfig.app.json', configJson => {
37+
configJson.files = ['main.ts'];
38+
}))
39+
.then(() => expectToFail(() => ng('build')))
40+
.then(({ message }) => {
41+
if (!message.includes('polyfills.ts is not part of the compilation')) {
42+
throw new Error(`Expected missing TS file error, got this instead:\n${message}`);
43+
}
44+
if (extraErrors.some((e) => message.includes(e))) {
45+
throw new Error(`Did not expect extra errors but got:\n${message}`);
46+
}
47+
})
48+
.then(() => updateJsonFile('./src/tsconfig.app.json', configJson => {
49+
configJson.files = undefined;
50+
}))
51+
// Check simple single syntax errors.
52+
// These shouldn't skip emit and just show a TS error.
53+
.then(() => appendToFile('./src/app/app.component.ts', ']]]'))
54+
.then(() => expectToFail(() => ng('build')))
55+
.then(({ message }) => {
56+
if (!message.includes('Declaration or statement expected.')) {
57+
throw new Error(`Expected syntax error, got this instead:\n${message}`);
58+
}
59+
if (extraErrors.some((e) => message.includes(e))) {
60+
throw new Error(`Did not expect extra errors but got:\n${message}`);
61+
}
62+
})
63+
.then(() => writeFile('./src/app/app.component.ts', origContent))
64+
// Check errors when files were not emitted.
65+
.then(() => writeFile('./src/app/app.component.ts', ''))
66+
.then(() => expectToFail(() => ng('build', '--aot')))
67+
.then(({ message }) => {
68+
if (!message.includes(`Unexpected value 'AppComponent`)) {
69+
throw new Error(`Expected static analysis error, got this instead:\n${message}`);
70+
}
71+
if (extraErrors.some((e) => message.includes(e))) {
72+
throw new Error(`Did not expect extra errors but got:\n${message}`);
73+
}
74+
})
75+
.then(() => writeFile('./src/app/app.component.ts', origContent));
76+
}

‎tests/e2e/tests/build/rebuild-error.ts

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,20 @@ import {
33
waitForAnyProcessOutputToMatch,
44
execAndWaitForOutputToMatch,
55
} from '../../utils/process';
6-
import {replaceInFile, appendToFile} from '../../utils/fs';
7-
import {getGlobalVariable} from '../../utils/env';
6+
import { replaceInFile, readFile, writeFile } from '../../utils/fs';
7+
import { getGlobalVariable } from '../../utils/env';
8+
import { wait } from '../../utils/utils';
89

910

1011
const failedRe = /webpack: Failed to compile/;
1112
const successRe = /webpack: Compiled successfully/;
13+
const extraErrors = [
14+
`Final loader didn't return a Buffer or String`,
15+
`doesn't contain a valid alias configuration`,
16+
`main.ts is not part of the TypeScript compilation.`,
17+
];
1218

13-
export default function() {
19+
export default function () {
1420
if (process.platform.startsWith('win')) {
1521
return Promise.resolve();
1622
}
@@ -19,16 +25,77 @@ export default function() {
1925
return Promise.resolve();
2026
}
2127

28+
// Skip in non-nightly tests. Switch this check around when ng5 is out.
29+
if (!getGlobalVariable('argv').nightly) {
30+
return Promise.resolve();
31+
}
32+
33+
let origContent: string;
34+
2235
return Promise.resolve()
23-
// Add an error to a non-main file.
24-
.then(() => appendToFile('src/app/app.component.ts', ']]]]]'))
36+
// Save the original contents of `./src/app/app.component.ts`.
37+
.then(() => readFile('./src/app/app.component.ts'))
38+
.then((contents) => origContent = contents)
39+
// Add a major error on a non-main file to the initial build.
40+
.then(() => writeFile('src/app/app.component.ts', ''))
2541
// Should have an error.
26-
.then(() => execAndWaitForOutputToMatch('ng', ['serve'], failedRe))
27-
// Fix the error, should trigger a rebuild.
42+
.then(() => execAndWaitForOutputToMatch('ng', ['serve', '--aot'], failedRe))
43+
.then((results) => {
44+
const stderr = results.stderr;
45+
if (!stderr.includes(`Unexpected value 'AppComponent`)) {
46+
throw new Error(`Expected static analysis error, got this instead:\n${stderr}`);
47+
}
48+
if (extraErrors.some((e) => stderr.includes(e))) {
49+
throw new Error(`Did not expect extra errors but got:\n${stderr}`);
50+
}
51+
})
52+
// Fix the error, should trigger a successful rebuild.
53+
.then(() => Promise.all([
54+
waitForAnyProcessOutputToMatch(successRe, 20000),
55+
writeFile('src/app/app.component.ts', origContent)
56+
]))
57+
.then(() => wait(2000))
58+
// Add an syntax error to a non-main file.
59+
// Build should still be successfull and error reported on forked type checker.
60+
.then(() => Promise.all([
61+
waitForAnyProcessOutputToMatch(successRe, 20000),
62+
writeFile('src/app/app.component.ts', origContent + '\n]]]]]')
63+
]))
64+
.then((results) => {
65+
const stderr = results[0].stderr;
66+
if (!stderr.includes('Declaration or statement expected.')) {
67+
throw new Error(`Expected syntax error, got this instead:\n${stderr}`);
68+
}
69+
if (extraErrors.some((e) => stderr.includes(e))) {
70+
throw new Error(`Did not expect extra errors but got:\n${stderr}`);
71+
}
72+
})
73+
// Fix the error, should trigger a successful rebuild.
2874
.then(() => Promise.all([
2975
waitForAnyProcessOutputToMatch(successRe, 20000),
3076
replaceInFile('src/app/app.component.ts', ']]]]]', '')
3177
]))
78+
.then(() => wait(2000))
79+
// Add a major error on a rebuild.
80+
// Should fail the rebuild.
81+
.then(() => Promise.all([
82+
waitForAnyProcessOutputToMatch(failedRe, 20000),
83+
writeFile('src/app/app.component.ts', '')
84+
]))
85+
.then((results) => {
86+
const stderr = results[0].stderr;
87+
if (!stderr.includes(`Unexpected value 'AppComponent`)) {
88+
throw new Error(`Expected static analysis error, got this instead:\n${stderr}`);
89+
}
90+
if (extraErrors.some((e) => stderr.includes(e))) {
91+
throw new Error(`Did not expect extra errors but got:\n${stderr}`);
92+
}
93+
})
94+
// Fix the error, should trigger a successful rebuild.
95+
.then(() => Promise.all([
96+
waitForAnyProcessOutputToMatch(successRe, 20000),
97+
writeFile('src/app/app.component.ts', origContent)
98+
]))
3299
.then(() => killAllProcesses(), (err: any) => {
33100
killAllProcesses();
34101
throw err;

‎tests/e2e/utils/utils.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11

2-
export function expectToFail(fn: () => Promise<any>, errorMessage?: string): Promise<void> {
2+
export function expectToFail(fn: () => Promise<any>, errorMessage?: string): Promise<any> {
33
return fn()
44
.then(() => {
55
const functionSource = fn.name || (<any>fn).source || fn.toString();
66
const errorDetails = errorMessage ? `\n\tDetails:\n\t${errorMessage}` : '';
77
throw new Error(
88
`Function ${functionSource} was expected to fail, but succeeded.${errorDetails}`);
9-
}, () => { });
9+
}, (err) => { return err; });
1010
}
1111

1212
export function wait(msecs: number): Promise<void> {

0 commit comments

Comments
 (0)