Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project control flow branches individually #54921

Closed

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Mar 18, 2024

Includes the following changes to allow the branches of if bloks to be projected individually and for case and default block contents to be projected.

fix(core): correctly project single-root content inside control flow

This commit changes the way we use containers to insert conditional content. Previously if and switch conditional would always use the first content as the insertion container. This strategy interfered with content projection that projects entire containers - as the consequence content for all cases would end up in slot matched by the first container. This could be very surprising as described in #54840

After the change each conditional content is projected into its own container. This means that content projection can match more than one container and result in correct display.

fix(compiler): capture all control flow branches for content projection in if blocks

Previously only the first branch of an if block was captured for content projection. This was done because of some planned refactors in the future. Since we've decided not to apply those refactors to conditionals, these changes update the compiler to capture each branch individually for content projection purposes.

fix(compiler): capture switch block cases for content projection

Captures the individual cases in switch blocks for content projection purposes.

refactor(compiler-cli): exit early when checking content projection

Updates the logic that detects if a node should be checked for control flow content projection to exit as soon as it detects a second root node, instead of counting the total and then checking if it's more than one.

Fixes #54840.

This commit changes the way we use containers to insert conditional content.
Previously if and switch conditional would always use the first content as the
insertion container. This strategy interfered with content projection that
projects entire containers - as the consequence content for _all_ cases would
end up in slot matched by the first container. This could be very surprising
as desicribed in angular#54840

After the change each conditional content is projected into its own container.
This means that content projection can match more than one container and
result in correct display.

Fixes angular#54840
…on in if blocks

Previously only the first branch of an `if` block was captured for content projection. This was done because of some planned refactors in the future. Since we've decided not to apply those refactors to conditionals, these changes update the compiler to capture each branch individually for content projection purposes.
Captures the individual cases in `switch` blocks for content projection purposes.
Updates the logic that detects if a node should be checked for control flow content projection to exit as soon as it detects a second root node, instead of counting the total and then checking if it's more than one.
@crisbeto crisbeto requested a review from dylhunn March 18, 2024 11:27
@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 Mar 18, 2024
@ngbot ngbot bot added this to the Backlog milestone Mar 18, 2024
@crisbeto crisbeto marked this pull request as ready for review March 18, 2024 11:28
@crisbeto crisbeto modified the milestones: Backlog, v18-candidates Mar 19, 2024
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

reviewed-for: fw-compiler

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 20, 2024
@dylhunn
Copy link
Contributor

dylhunn commented Mar 22, 2024

This PR was merged into the repository by commit fc13144.

@dylhunn dylhunn closed this in 1c0ec56 Mar 22, 2024
dylhunn pushed a commit that referenced this pull request Mar 22, 2024
…on in if blocks (#54921)

Previously only the first branch of an `if` block was captured for content projection. This was done because of some planned refactors in the future. Since we've decided not to apply those refactors to conditionals, these changes update the compiler to capture each branch individually for content projection purposes.

PR Close #54921
dylhunn pushed a commit that referenced this pull request Mar 22, 2024
)

Captures the individual cases in `switch` blocks for content projection purposes.

PR Close #54921
dylhunn pushed a commit that referenced this pull request Mar 22, 2024
…54921)

Updates the logic that detects if a node should be checked for control flow content projection to exit as soon as it detects a second root node, instead of counting the total and then checking if it's more than one.

PR Close #54921
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
…ngular#54921)

This commit changes the way we use containers to insert conditional content.
Previously if and switch conditional would always use the first content as the
insertion container. This strategy interfered with content projection that
projects entire containers - as the consequence content for _all_ cases would
end up in slot matched by the first container. This could be very surprising
as desicribed in angular#54840

After the change each conditional content is projected into its own container.
This means that content projection can match more than one container and
result in correct display.

Fixes angular#54840

PR Close angular#54921
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
…on in if blocks (angular#54921)

Previously only the first branch of an `if` block was captured for content projection. This was done because of some planned refactors in the future. Since we've decided not to apply those refactors to conditionals, these changes update the compiler to capture each branch individually for content projection purposes.

PR Close angular#54921
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
…ular#54921)

Captures the individual cases in `switch` blocks for content projection purposes.

PR Close angular#54921
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
…ngular#54921)

Updates the logic that detects if a node should be checked for control flow content projection to exit as soon as it detects a second root node, instead of counting the total and then checking if it's more than one.

PR Close angular#54921
@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 Apr 22, 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.

Conditionals and content projection
3 participants