Skip to content

fix(compiler-cli): don't type check the bodies of control flow nodes in basic mode#55360

Closed
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:52969/control-flow-body-check
Closed

fix(compiler-cli): don't type check the bodies of control flow nodes in basic mode#55360
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:52969/control-flow-body-check

Conversation

@crisbeto
Copy link
Member

Angular only checks the contents of template nodes in full type checking mode. After v17, the new control flow always had its body checked, even in basic mode, which started revealing compilation errors for apps that were using the schematic to automatically switch to the new syntax.

These changes mimic the old behavior by not checking the bodies of if, switch and for blocks in basic mode. Note that the expressions of the blocks are still going to be checked.

Fixes #52969.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release area: compiler Issues related to `ngc`, Angular's template compiler labels Apr 16, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 16, 2024
@crisbeto crisbeto force-pushed the 52969/control-flow-body-check branch from 2f052fa to 6ed2f1b Compare April 16, 2024 08:41
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was debating whether to reuse checkTemplateBodies instead of introducing this one. I decided to do it since it's a bit more explicit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can go either way. I wouldn't personally have introduced it, but there's no harm in keeping it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically we don't need to skip the guard here since no children will be generated hence nothing will use it. I added the check here to avoid some of the work when we can.

@crisbeto crisbeto force-pushed the 52969/control-flow-body-check branch from 6ed2f1b to ab4283c Compare April 16, 2024 08:49
@crisbeto crisbeto changed the title fix(compiler-cli): don't the bodies of control flow nodes in basic mode fix(compiler-cli): don't type check the bodies of control flow nodes in basic mode Apr 16, 2024
@crisbeto crisbeto requested a review from JoostK April 16, 2024 08:59
@crisbeto crisbeto marked this pull request as ready for review April 16, 2024 08:59
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One request for changing a test, other than that LGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can go either way. I wouldn't personally have introduced it, but there's no harm in keeping it.

@JoostK JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 17, 2024
…in basic mode

Angular only checks the contents of template nodes in full type checking mode. After v17, the new control flow always had its body checked, even in basic mode, which started revealing compilation errors for apps that were using the schematic to automatically switch to the new syntax.

These changes mimic the old behavior by not checking the bodies of `if`, `switch` and `for` blocks in basic mode. Note that the expressions of the blocks are still going to be checked.

Fixes angular#52969.
@crisbeto crisbeto force-pushed the 52969/control-flow-body-check branch from ab4283c to 2d06014 Compare April 18, 2024 11:29
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 18, 2024
@alxhub
Copy link
Member

alxhub commented Apr 19, 2024

This PR was merged into the repository by commit 7a16d7e.

@alxhub alxhub closed this in 7a16d7e Apr 19, 2024
@minijus
Copy link

minijus commented Apr 25, 2024

Could this fix be included in v17 as well?

@crisbeto
Copy link
Member Author

v17 port is in #55558.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

After running the control flow migration strict template checking is enforced.

4 participants