fix(compiler-cli): skip `ExtendedTemplateCheckerImpl` construction if there were configuration errors by dgp1130 · Pull Request #44778 · angular/angular

dgp1130 · 2022-01-20T19:32:28Z

Previously, if a bad extended diagnostic category was given, it would fail with the expected error as well as an unexpected assertion error:

$ ng build -c development
✔ Browser application bundle generation complete.

./src/main.ts - Error: Module build failed (from ./node_modules/@ngtools/webpack/src/ivy/index.js):
Error: Unexpected call to 'assertNever()' with value:
test
    at /home/douglasparker/Source/ng-new/node_modules/@ngtools/webpack/src/ivy/loader.js:77:18
    at processTicksAndRejections (internal/process/task_queues.js:95:5)

./src/polyfills.ts - Error: Module build failed (from ./node_modules/@ngtools/webpack/src/ivy/index.js):
Error: Unexpected call to 'assertNever()' with value:
test
    at /home/douglasparker/Source/ng-new/node_modules/@ngtools/webpack/src/ivy/loader.js:77:18
    at processTicksAndRejections (internal/process/task_queues.js:95:5)

Error: error NG4004: Angular compiler option "extendedDiagnostics.checks['invalidBananaInBox']" has an unknown diagnostic category: "test".

Allowed diagnostic categories are:
warning
error
suppress

The assertion comes from ExtendedTemplateCheckerImpl, which expects a well-formed configuration, yet the compiler would construct it even when errors were found. This commit skips constructing and running extended diagnostics if the configuration had errors, which should avoid triggering these assertion errors.

I'm unfortunately not able to actually test this change. The test passes even before the fix because the ngc binary and end-to-end tests don't request diagnostics unless the configuration is considered valid. See Slack for more details.

I'm pretty sure this is the right fix, but if you have any suggestions for how to test / verify this before releasing it, I'm happy to do what I can.

I listed this as target: rc since it would be nice to get it in for 13.2.0, but functionally it is just a bad error message, so if we can't get it in and need to wait for a patch release, I'm ok with that too.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

Issue Number: #42966

Using a bad diagnostic category in tsconfig.json for extended diagnostics throws the expected error and also some assertion errors which clutter the output.

What is the new behavior?

Using a bad diagnostic category in tsconfig.json for extended diagnostics only throws the expected error and no assertion errors.

Does this PR introduce a breaking change?

No

Other information

dgp1130 · 2022-01-20T19:41:07Z

Updated some language service tests to fix CI. Now they check that strictTemplates: false will not emit diagnostics, as they were previously relying on not passing _extendedTemplateDiagnostics.

AndrewKushnir · 2022-01-20T22:05:17Z

@dgp1130 there PR has a merge conflict with the main branch, could you please rebase and resolve conflicts when you get a chance? Thank you.

… there were configuration errors Previously, if a bad extended diagnostic category was given, it would fail with the expected error as well as an unexpected assertion error: ``` $ ng build -c development ✔ Browser application bundle generation complete. ./src/main.ts - Error: Module build failed (from ./node_modules/@ngtools/webpack/src/ivy/index.js): Error: Unexpected call to 'assertNever()' with value: test at /home/douglasparker/Source/ng-new/node_modules/@ngtools/webpack/src/ivy/loader.js:77:18 at processTicksAndRejections (internal/process/task_queues.js:95:5) ./src/polyfills.ts - Error: Module build failed (from ./node_modules/@ngtools/webpack/src/ivy/index.js): Error: Unexpected call to 'assertNever()' with value: test at /home/douglasparker/Source/ng-new/node_modules/@ngtools/webpack/src/ivy/loader.js:77:18 at processTicksAndRejections (internal/process/task_queues.js:95:5) Error: error NG4004: Angular compiler option "extendedDiagnostics.checks['invalidBananaInBox']" has an unknown diagnostic category: "test". Allowed diagnostic categories are: warning error suppress ``` The assertion comes from `ExtendedTemplateCheckerImpl`, which expects a well-formed configuration, yet the compiler would construct it even when errors were found. This commit skips constructing and running extended diagnostics if the configuration had errors, which should avoid triggering these assertion errors. I'm unfortunately not able to actually test this change. The test passes even before the fix because the `ngc` binary and end-to-end tests [don't request diagnostics unless the configuration is considered valid](https://github.com/angular/angular/blob/ed21f5c75378e1ce717ee3d76d28c8c994209de1/packages/compiler-cli/src/perform_compile.ts#L292-L293). See [Slack](https://angular-team.slack.com/archives/C4WHZQMRA/p1642641305003800) for more details.

dgp1130 · 2022-01-21T00:03:04Z

@AndrewKushnir, sorry about that, I figured this would conflict with my other PR. Should be fixed now, and fortunately it helped me notice that I screwed up the commit history as well. Good now, just waiting for CI I guess.

AndrewKushnir · 2022-01-21T19:20:51Z

This PR was merged into the repository by commit d8bff1a.

… there were configuration errors (#44778) Previously, if a bad extended diagnostic category was given, it would fail with the expected error as well as an unexpected assertion error: ``` $ ng build -c development ✔ Browser application bundle generation complete. ./src/main.ts - Error: Module build failed (from ./node_modules/@ngtools/webpack/src/ivy/index.js): Error: Unexpected call to 'assertNever()' with value: test at /home/douglasparker/Source/ng-new/node_modules/@ngtools/webpack/src/ivy/loader.js:77:18 at processTicksAndRejections (internal/process/task_queues.js:95:5) ./src/polyfills.ts - Error: Module build failed (from ./node_modules/@ngtools/webpack/src/ivy/index.js): Error: Unexpected call to 'assertNever()' with value: test at /home/douglasparker/Source/ng-new/node_modules/@ngtools/webpack/src/ivy/loader.js:77:18 at processTicksAndRejections (internal/process/task_queues.js:95:5) Error: error NG4004: Angular compiler option "extendedDiagnostics.checks['invalidBananaInBox']" has an unknown diagnostic category: "test". Allowed diagnostic categories are: warning error suppress ``` The assertion comes from `ExtendedTemplateCheckerImpl`, which expects a well-formed configuration, yet the compiler would construct it even when errors were found. This commit skips constructing and running extended diagnostics if the configuration had errors, which should avoid triggering these assertion errors. I'm unfortunately not able to actually test this change. The test passes even before the fix because the `ngc` binary and end-to-end tests [don't request diagnostics unless the configuration is considered valid](https://github.com/angular/angular/blob/ed21f5c75378e1ce717ee3d76d28c8c994209de1/packages/compiler-cli/src/perform_compile.ts#L292-L293). See [Slack](https://angular-team.slack.com/archives/C4WHZQMRA/p1642641305003800) for more details. PR Close #44778

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fanimations/13.1.2/13.2.1) | | [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcommon/13.1.2/13.2.1) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/13.1.2/13.2.1) | | [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/13.1.2/13.2.1) | | [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcore/13.1.2/13.2.1) | | [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fforms/13.1.2/13.2.1) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/13.1.2/13.2.1) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/13.1.2/13.2.1) | | [@angular/router](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2frouter/13.1.2/13.2.1) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v13.2.1`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1321-2022-02-02) [Compare Source](angular/angular@13.2.0...13.2.1) ##### animations | Commit | Type | Description | | -- | -- | -- | | [4644886aaf](angular/angular@4644886) | perf | remove no longer needed CssKeyframes classes ([#44903](angular/angular#44903)) ([#44919](angular/angular#44919)) | ##### common | Commit | Type | Description | | -- | -- | -- | | [b4e4617807](angular/angular@b4e4617) | fix | include query parameters for open HTTP requests in `verify` ([#44917](angular/angular#44917)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [0778e6f7d7](angular/angular@0778e6f) | fix | accept nullish coalescing operator for any and unknown types ([#44862](angular/angular#44862)) | | [07185f4ed1](angular/angular@07185f4) | fix | enable nullish coalescing check only with `strictNullChecks` ([#44862](angular/angular#44862)) | | [4a5ad1793f](angular/angular@4a5ad17) | fix | ensure casing of logical paths is preserved ([#44798](angular/angular#44798)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [7ec482d9c2](angular/angular@7ec482d) | fix | Add back support for namespace URIs in createElement of dom renderer ([#44914](angular/angular#44914)) | | [250dc40a46](angular/angular@250dc40) | fix | flush delayed scoping queue while setting up TestBed ([#44814](angular/angular#44814)) | ##### forms | Commit | Type | Description | | -- | -- | -- | | [1aebbf8714](angular/angular@1aebbf8) | fix | ensure OnPush ancestors are marked dirty when the promise resolves ([#44886](angular/angular#44886)) | | [6b7fffcbeb](angular/angular@6b7fffc) | fix | Update the typed forms migration schematic to find all files. ([#44881](angular/angular#44881)) | #### Special Thanks Alan, Andrew Kushnir, Andrew Scott, Aristeidis Bampakos, Arjen, Daniel Díaz, David Shevitz, Doug Parker, Dylan Hunn, Esteban Gehring, George Kalpakas, Jessica Janiuk, JoostK, Juri Strumpflohner, Lee Robinson, Maarten Tibau, Paul Gschwendtner, Theodore Brown, arturovt, dario-piotrowicz, fru2, markostanimirovic and mgechev  ### [`v13.2.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1320-2022-01-26) [Compare Source](angular/angular@13.1.3...13.2.0) #### Deprecations ##### - The `CachedResourceLoader` and `RESOURCE_CACHE_PROVIDER` symbols were previously necessary in some cases to test AOT-compiled components with View Engine, but they are no longer needed since Ivy. - The `ComponentFactory` and `ComponentFactoryResolver` classes are deprecated. Since Ivy, there is no need to resolve Component factories. Please use other APIs where you Component classes can be used directly (without resolving their factories). - Since Ivy, the `CompilerOptions.useJit` and `CompilerOptions.missingTranslation` config options are unused, passing them has no effect. ##### | Commit | Type | Description | | -- | -- | -- | | [9c11183e74](angular/angular@9c11183) | docs | deprecate `CachedResourceLoader` and `RESOURCE_CACHE_PROVIDER` symbols ([#44749](angular/angular#44749)) | | [9f12e7fea4](angular/angular@9f12e7f) | docs | deprecate `ComponentFactory` and `ComponentFactoryResolver` symbols ([#44749](angular/angular#44749)) | | [4e95a316ce](angular/angular@4e95a31) | docs | deprecate unused config options from the `CompilerOptions` interface ([#44749](angular/angular#44749)) | ##### compiler | Commit | Type | Description | | -- | -- | -- | | [a4ab6d6b72](angular/angular@a4ab6d6) | feat | add support for safe calls in templates ([#44580](angular/angular#44580)) | | [abd1bc8039](angular/angular@abd1bc8) | fix | correct spans when parsing bindings with comments ([#44785](angular/angular#44785)) | | [ed67a074ce](angular/angular@ed67a07) | fix | properly compile DI factories when coverage reporting is enabled ([#44732](angular/angular#44732)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [fa835b5a29](angular/angular@fa835b5) | feat | enable extended diagnostics by default ([#44712](angular/angular#44712)) | | [73424def13](angular/angular@73424de) | feat | provide the animations for `DirectiveMeta` ([#44630](angular/angular#44630)) | | [fe3e4d6865](angular/angular@fe3e4d6) | fix | Handle `ng-template` with structural directive in indexer ([#44788](angular/angular#44788)) | | [7316e72ec5](angular/angular@7316e72) | fix | properly index <svg> elements when on a template ([#44785](angular/angular#44785)) | | [100091ebf0](angular/angular@100091e) | fix | remove leftover `_extendedTemplateDiagnostics` requirements ([#44777](angular/angular#44777)) | | [d2ae96f742](angular/angular@d2ae96f) | fix | skip `ExtendedTemplateCheckerImpl` construction if there were configuration errors ([#44778](angular/angular#44778)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [5626b34264](angular/angular@5626b34) | fix | consistently use namespace short name rather than URI ([#44766](angular/angular#44766)) | | [94bfcdd9de](angular/angular@94bfcdd) | fix | error if NgZone.isInAngularZone is called with a noop zone ([#44800](angular/angular#44800)) | ##### forms | Commit | Type | Description | | -- | -- | -- | | [72092ebd26](angular/angular@72092eb) | feat | Allow a FormControl to use initial value as default. ([#44434](angular/angular#44434)) | | [f7aa937cac](angular/angular@f7aa937) | fix | Make some minor fixups for forward-compatibility with typed forms. ([#44540](angular/angular#44540)) | ##### router | Commit | Type | Description | | -- | -- | -- | | [5a4ddfd4f5](angular/angular@5a4ddfd) | feat | Allow symbol keys for `Route` `data` and `resolve` properties ([#44519](angular/angular#44519)) | #### Special Thanks Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Dario Piotrowicz, Derek Cormier, Doug Parker, Douglas Parker, Dylan Hunn, George Kalpakas, Jessica Janiuk, JoostK, Kristiyan Kostadinov, Martin Probst, Oleg Postoev, Stephanie Tuerk, Tim Bowersox, Wiley Marques, Yousaf Nawaz, dario-piotrowicz, iRealNirmal, ivanwonder and shejialuo  ### [`v13.1.3`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1313-2022-01-19) [Compare Source](angular/angular@13.1.2...13.1.3) ##### animations | Commit | Type | Description | | -- | -- | -- | | [af0a152a2c](angular/angular@af0a152) | fix | apply setStyles to only rootTimelines ([#44515](angular/angular#44515)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [626f3f230b](angular/angular@626f3f2) | perf | reduce analysis work during incremental rebuilds ([#44731](angular/angular#44731)) | ##### ngcc | Commit | Type | Description | | -- | -- | -- | | [f9ca4d8499](angular/angular@f9ca4d8) | fix | support element accesses for export declarations ([#44669](angular/angular#44669)) | #### Special Thanks Alan Agius, Andrew Kushnir, AnkitSharma-007, Daniel Díaz, Dmytro Mezhenskyi, Jessica Janiuk, Joey Perrott, JoostK, Ramesh Thiruchelvam, dario-piotrowicz, iRealNirmal and Łukasz Holeczek  </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Co-authored-by: crapStone <crapstone@noreply.codeberg.org> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1148 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2022-02-21T16:06:24Z

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.}

dgp1130 added area: compiler Issues related to `ngc`, Angular's template compiler target: rc This PR is targeted for the next release-candidate labels Jan 20, 2022

dgp1130 requested review from alxhub and atscott January 20, 2022 19:32

ngbot bot added this to the Backlog milestone Jan 20, 2022

dgp1130 force-pushed the etd-assertion branch from a0ff3a6 to 70fd06d Compare January 20, 2022 19:43

atscott approved these changes Jan 20, 2022

View reviewed changes

dgp1130 removed the request for review from alxhub January 20, 2022 21:59

dgp1130 added the action: merge The PR is ready for merge by the caretaker label Jan 20, 2022

AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 20, 2022

dgp1130 force-pushed the etd-assertion branch from 70fd06d to a8199d5 Compare January 20, 2022 23:58

dgp1130 removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 21, 2022

AndrewKushnir closed this in d8bff1a Jan 21, 2022

dgp1130 deleted the etd-assertion branch January 21, 2022 19:21

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler-cli): skip `ExtendedTemplateCheckerImpl` construction if there were configuration errors#44778

fix(compiler-cli): skip `ExtendedTemplateCheckerImpl` construction if there were configuration errors#44778
dgp1130 wants to merge 1 commit intoangular:masterfrom
dgp1130:etd-assertion

dgp1130 commented Jan 20, 2022

Uh oh!

dgp1130 commented Jan 20, 2022

Uh oh!

AndrewKushnir commented Jan 20, 2022

Uh oh!

dgp1130 commented Jan 21, 2022

Uh oh!

AndrewKushnir commented Jan 21, 2022

Uh oh!

angular-automatic-lock-bot bot commented Feb 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dgp1130 commented Jan 20, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

dgp1130 commented Jan 20, 2022

Uh oh!

AndrewKushnir commented Jan 20, 2022

Uh oh!

dgp1130 commented Jan 21, 2022

Uh oh!

AndrewKushnir commented Jan 21, 2022

Uh oh!

angular-automatic-lock-bot bot commented Feb 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants