Inconsistency behavior when handling errors

[Fatme]

🐞 Bug report

Command (mark with an x)

- [ ] new
- [ ] build
- [ x ] serve
- [ x ] test
- [ ] e2e
- [ ] generate
- [ ] add
- [ ] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Is this a regression?

No.

Description

Handling only syntax errors when it is not a first run

It seems that AngularCompilerPlugin handles both syntactic and semantic errors on first run, but it handles only syntax errors when it is not the first run.

angular-cli/packages/ngtools/webpack/src/angular_compiler_plugin.ts

Lines 1272 to 1273 in 95776fe

	const diagMode = (this._firstRun || !this._forkTypeChecker) ?
	DiagnosticMode.All : DiagnosticMode.Syntactic;

This lead to the following inconsistency behavior when executing ng serve:

First run

1. Add the code below to app.component.ts:

public ngOnInit() {
    const x:number = '10';
    console.log('============ x = ', x);
}

2. Execute ng serve

Actual behavior:

ERROR in src/app/app.component.ts:12:11 - error TS2322: Type '"10"' is not assignable to type 'number'.

12     const x:number = '10';
｢wdm｣: Failed to compile.

Not a first run

1. Execute ng serve
2. Add the code below to app.component.ts:

public ngOnInit() {
    const x:number = '10';
    console.log('============ x = ', x);
}

Actual behavior:

wdm｣: Compiled successfully.
    
    ERROR in src/app/app.component.ts(12,11): error TS2322: Type '"10"' is not assignable to type 'number'.

As it can be seen, the webpack compilation is not successful on first run but it is fine when it is not the first run. I'm wondering if this is the intended behavior and what is the reason for that behavior.

Currently we're relying on AngularCompilerPlugin and in case it is intended behavior, is it possible to introduce an option so the diagnostics mode can be forced? That will give us the ability to handle both syntax and semantic errors regardless if it is first run or not.

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

    _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 8.3.12
Node: 10.13.0
OS: darwin x64
Angular: 8.2.11
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.803.12
@angular-devkit/build-angular     0.803.12
@angular-devkit/build-optimizer   0.803.12
@angular-devkit/build-webpack     0.803.12
@angular-devkit/core              8.3.12
@angular-devkit/schematics        8.3.12
@angular/cli                      8.3.12
@ngtools/webpack                  8.3.12
@schematics/angular               8.3.12
@schematics/update                0.803.12
rxjs                              6.4.0
typescript                        3.5.3
webpack                           4.39.2

Anything else relevant?

[alan-agius4]

Hi @Fatme,

Yes, it's intended that type errors aren't sent to Webpack when using the forked type checker. If they were, and they blocked the compilation, then there's no reason to in having a forked type checker turned on as rebuilds would just take much longer.

[Fatme]

Hey @alan-agius4,

Yes, it's intended that type errors aren't sent to Webpack when using the forked type checker.

As far as I understand you, you mean that it is intentional not stopping on type errors.

A time ago we opened a PR that respects noEmitOnError typescript’s compiler option when emitting the code. The PR was closed due to the following reason:

 In this case, the team believes that correcting any errors within the application would be the more beneficial and prudent action in both the short and long term.

But if the incremental compilation is never stopping on type errors, how the users can correct their errors within the application?

So, it's really confusing why the type errors are respected only on first run. Shouldn't the behavior be unified for both cases - first run and incremental compilation?

[Fatme]

Hey @alan-agius4,

We discussed the problem with the team and we have the following suggestion:

What about introducing asyncTypeChecking option to AngularCompilerPlugin? This option will control if the errors reported by forked type checking process will be integrated with the webpack compilation. The option will have the following values:

• true -> this will be the default value and it'll not change the current behavior of error handling of the incremental webpack compilations.
• false -> this will block webpack's afterCompile hook to wait for type checker/linter and to add errors to the webpack's compilation

Let us know about your opinion on that matter.

[clydin]

An option for that already exists: forkTypeChecker. Set it to false and the type checking will then be done inline with the TypeScript compilation. However, as previously noted, rebuild times will increase when using ng serve or watch mode.

[Fatme]

Hey @clydin,

An option for that already exists: forkTypeChecker

This option is different from the proposed asyncTypeChecking option. The forkTypeChecker option controls if a separate type checking process will be ran or an inline type checking will be executed with TypeScript compilation. The asyncTypeChecking will control if the already started separate type checking process will be awaited from webpack compilation. It will be something similar to async option of fork-ts-checker-plugin

the type checking will then be done inline with the TypeScript compilation

No, the type checking will be done from the forked process, webpack compilation will be blocked on afterCompile hook until we receive a response from the forked process in case when asyncTypeChecking is false. This way when noEmitOnErrors is set to true within webpack.config.js, webpack will stop the execution and will print that the compilation is not successful.

This will be beneficial when AngularCompilerPlugin is used as a standalone plugin within webpack.config.js. And in the same time, it'll not break neither already existing applications nor newly created apps as the default value of asyncTypeChecking will be true.

[Fatme]

Ping @alan-agius4 @clydin.

[alan-agius4]

Personally, I am not entirely sure how beneficial it would to be to have an async forked type checking vs inline type checking in the same process. Even then, considering that most users are more interested in having the shortest possible incremental builds rather than not a compilation marked as not successful during rebuild. I am not sure if this is worth the future maintenance and support that it will need.

@clydin what do you think?

[Fatme]

Hey @alan-agius4,

Even then, considering that most users are more interested in having the shortest possible incremental builds rather than not a compilation marked as not successful during rebuild.

We have the opposite case - our users want to know when the compilation is not successful rather than having a runtime crashed mobile application as it is much harder for debugging and investigating what exactly caused the problem.

I am not sure if this is worth the future maintenance and support that it will need.

I think that the future maintenance will be needed only in case when updating to the next major version of webpack. It shouldn't be affected by other changes and should not affect other parts of the code. It should be used mainly when AngularCompilerPlugin is used as a standalone webpack plugin where the user has control over the provided options.

The feature is really important for our error handling story aiming to improve the current development experience of our users.

[filipesilva]

It is true that we could feed errors back from the type checker into the main compilation process, blocking compilation until they get there. But I don't see what the purpose is when it's already possible to wholly disable the type checker via the forkTypeChecker option.

The main purpose of the forked is to reduce the rebuild time, at the cost of type correctness. To be fair it is always jarring to have type errors come after compilation is supposedly done, and it's not a great user experience when there are errors. It's pretty ok when there are no errors though, then it's just a net improvement.

The resource strain can also be considerable on AOT compilations, and the forked type checker always uses a lower fidelity compilation since it doesn't have the full webpack pipeline behind it.

The proposed asyncTypeChecking could help in cases where the base compilation (without type checking) finishes, and then webpack is still doing other stuff while the type checker does its work. In that case we would get two processes doing work. That's very common on first builds (where we don't fork the type checker at all), and very uncommon on rebuilds (the main purpose of the type checker in the first place).

So to me adding asyncTypeChecking just sounds like new semantics, development time, maintenance cost, new failure modes, and test cases to essentially get the same as forkTypeChecker: false but using a lot more resources.

I understand that it's possible, but I don't see the cases where it's desirable. Do you have cases where you think asyncTypeChecking would be superior to disabling the forked type checker?