Repository navigation
Inconsistency behavior when handling errors #15887
Description
🐞 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
This lead to the following inconsistency behavior when executing ng serve:
First run
- Add the code below to
app.component.ts:
public ngOnInit() {
const x:number = '10';
console.log('============ x = ', x);
}
- 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
- Execute
ng serve - 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?
Activity
alan-agius4 commentedon Oct 21, 2019
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 commentedon Oct 23, 2019
Hey @alan-agius4,
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:
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 commentedon Oct 24, 2019
Hey @alan-agius4,
We discussed the problem with the team and we have the following suggestion:
What about introducing
asyncTypeCheckingoption 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:Let us know about your opinion on that matter.
clydin commentedon Oct 24, 2019
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 usingng serveor watch mode.Fatme commentedon Oct 24, 2019
Hey @clydin,
This option is different from the proposed
asyncTypeCheckingoption. TheforkTypeCheckeroption controls if a separate type checking process will be ran or an inline type checking will be executed with TypeScript compilation. TheasyncTypeCheckingwill 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-pluginNo, 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
asyncTypeCheckingis false. This way whennoEmitOnErrorsis set to true withinwebpack.config.js, webpack will stop the execution and will print that the compilation is not successful.This will be beneficial when
AngularCompilerPluginis 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 ofasyncTypeCheckingwill be true.Fatme commentedon Oct 26, 2019
Ping @alan-agius4 @clydin.
alan-agius4 commentedon Oct 28, 2019
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 commentedon Nov 6, 2019
Hey @alan-agius4,
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 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
AngularCompilerPluginis 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 commentedon Nov 14, 2019
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
forkTypeCheckeroption.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
asyncTypeCheckingcould 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
asyncTypeCheckingjust sounds like new semantics, development time, maintenance cost, new failure modes, and test cases to essentially get the same asforkTypeChecker: falsebut 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
asyncTypeCheckingwould be superior to disabling the forked type checker?angular-automatic-lock-bot commentedon Dec 6, 2020
This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.
Read more about our automatic conversation locking policy.
This action has been performed automatically by a bot.