Project root node of @if and @for blocks #52414

crisbeto · 2023-10-27T08:03:52Z

A couple of fixes to get the content projection behavior of @if and @for blocks closer to *ngIf and *ngFor. Includes the following commits:

fix(compiler): project control flow root elements into correct slot

With the directive-based control flow users were able to conditionally project content using the * syntax. E.g. <div *ngIf="expr" projectMe></div> will be projected into <ng-content select="[projectMe]"/>, because the attributes and tag name from the div are copied to the template via the template creation instruction. With @if and @for that is not the case, because the conditional is placed around elements, rather than on them. The result is that content projection won't work in the same way if a user converts from *ngIf to @if.

These changes aim to cover the most common case by doing the same copying when a control flow node has one and only one root element or template node.

This approach comes with some caveats:

As soon as any other node is added to the root, the copying behavior won't work anymore. A diagnostic will be added to flag cases like this and to explain how to work around it.
If preserveWhitespaces is enabled, it's very likely that indentation will break this workaround, because it'll include an additional text node as the first child. We can work around it here, but in a discussion it was decided not to, because the user explicitly opted into preserving the whitespace and we would have to drop it from the generated code. The diagnostic mentioned point 1 will flag such cases to users.

refactor(compiler): implement control flow content projection fix in template pipeline

Recreates the fix for content projection in control flow in the new template pipeline. I also had to make the following adjustments to the pipeline:

The TemplateOp.tag property was being used to generate the name of the template function, rather than the actual tag name being passed into ɵɵtemplate. Since the content projection fix requires the tag name to be passed in, I've introduced a new functionNameSuffix property instead.
TemplateOp.block was being used to determine whether to pass TemplateOp.tag into the ɵɵtemplate instruction. Now that we're always passing in the tag name after the refactor in point 1, we no longer need this flag.

In addition to the refactors above, I also made some minor cleanups where I saw the opportunity to do so.

Fixes #52277.

crisbeto · 2023-10-27T08:04:39Z

packages/compiler/src/template/pipeline/ir/src/ops/create.ts

   */
-  block: boolean;
+  functionNameSuffix: string|null;


I'm not super attached to the name so I'm open to suggestions. The main goal was to stop using the tag name for the function name generation.

packages/compiler/src/template/pipeline/src/ingest.ts

packages/compiler/src/template/pipeline/src/instruction.ts

goldens/size-tracking/aio-payloads.json

crisbeto · 2023-10-30T08:56:33Z

I've pushed some changes to avoid future issues if we start capturing comments by default and to rebase.

pkozlowski-opensource

LGTM overall but I would like to revisit this assumption:

If preserveWhitespaces is enabled, it's very likely that indentation will break this

workaround, because it'll include an additional text node as the first child. We can work
around it here, but in a discussion it was decided not to, because the user explicitly opted
into preserving the whitespace and we would have to drop it from the generated code.
The diagnostic mentioned point transpiler: show line numbers that have errors #1 will flag such cases to users.

I would say that we should have just one rule: we opt out of the root node capturing if there is more than one root node. This will be more common case with preserveWhitespaces but still there is one consistent rule here.

packages/core/test/acceptance/control_flow_for_spec.ts

crisbeto · 2023-10-30T10:22:58Z

Feedback has been addressed.

pkozlowski-opensource

LGTM, thnx!

dylhunn

LGTM. Makes perfect sense to me, and thank you for also updating the template pipeline. A lot of the use of tag for control flow and conditionals was marginal to begin with -- it was more designed for elements and containers.

packages/compiler/src/template/pipeline/src/ingest.ts

packages/compiler/src/template/pipeline/src/instruction.ts

dylhunn · 2023-10-30T16:52:00Z

packages/compiler/src/template/pipeline/src/phases/naming.ts

              state, compatibility);
        }
-        const repeaterToken =
-            op.tag === null ? '' : '_' + prefixWithNamespace(op.tag, op.namespace);


I kind of preferred having this prefixing logic in the naming phase as opposed to ingest, where you've now folded it into the new suffix field -- it seems like it's logically part of naming. It's a weak preference though. I don't think we could conceivably need to use the raw suffix for anything else, so it's probably fine?

I agree that it would've felt more appropriate to have the naming logic there, but the rules we have for the function naming are a bit too different for the template nodes vs control flow that I couldn't find a better place than this one. I think that once we remove the definition builder, we can revisit the naming scheme.

With the directive-based control flow users were able to conditionally project content using the `*` syntax. E.g. `<div *ngIf="expr" projectMe></div>` will be projected into `<ng-content select="[projectMe]"/>`, because the attributes and tag name from the `div` are copied to the template via the template creation instruction. With `@if` and `@for` that is not the case, because the conditional is placed *around* elements, rather than *on* them. The result is that content projection won't work in the same way if a user converts from `*ngIf` to `@if`. These changes aim to cover the most common case by doing the same copying when a control flow node has *one and only one* root element or template node. This approach comes with some caveats: 1. As soon as any other node is added to the root, the copying behavior won't work anymore. A diagnostic will be added to flag cases like this and to explain how to work around it. 2. If `preserveWhitespaces` is enabled, it's very likely that indentation will break this workaround, because it'll include an additional text node as the first child. We can work around it here, but in a discussion it was decided not to, because the user explicitly opted into preserving the whitespace and we would have to drop it from the generated code. The diagnostic mentioned point angular#1 will flag such cases to users. Fixes angular#52277.

…template pipeline Recreates the fix for content projection in control flow in the new template pipeline. I also had to make the following adjustments to the pipeline: 1. The `TemplateOp.tag` property was being used to generate the name of the template function, rather than the actual tag name being passed into `ɵɵtemplate`. Since the content projection fix requires the tag name to be passed in, I've introduced a new `functionNameSuffix` property instead. 2. `TemplateOp.block` was being used to determine whether to pass `TemplateOp.tag` into the `ɵɵtemplate` instruction. Now that we're always passing in the tag name after the refactor in point 1, we no longer need this flag. In addition to the refactors above, I also made some minor cleanups where I saw the opportunity to do so.

crisbeto · 2023-10-31T20:54:17Z

Caretaker note: the aio-local failure is unrelated and is present in the main branch as well.

alxhub · 2023-10-31T21:52:33Z

This PR was merged into the repository by commit c7b730e.

…52414) With the directive-based control flow users were able to conditionally project content using the `*` syntax. E.g. `<div *ngIf="expr" projectMe></div>` will be projected into `<ng-content select="[projectMe]"/>`, because the attributes and tag name from the `div` are copied to the template via the template creation instruction. With `@if` and `@for` that is not the case, because the conditional is placed *around* elements, rather than *on* them. The result is that content projection won't work in the same way if a user converts from `*ngIf` to `@if`. These changes aim to cover the most common case by doing the same copying when a control flow node has *one and only one* root element or template node. This approach comes with some caveats: 1. As soon as any other node is added to the root, the copying behavior won't work anymore. A diagnostic will be added to flag cases like this and to explain how to work around it. 2. If `preserveWhitespaces` is enabled, it's very likely that indentation will break this workaround, because it'll include an additional text node as the first child. We can work around it here, but in a discussion it was decided not to, because the user explicitly opted into preserving the whitespace and we would have to drop it from the generated code. The diagnostic mentioned point #1 will flag such cases to users. Fixes #52277. PR Close #52414

…rol flow (#52515) Discovered this while validating #52414 against Angular Material. We were projecting `<ng-template>` nodes at the root of `@if` and `@for` with the `ng-template` tag name which enables directive matching and applies the directive to the control flow node. These changes fix the issue by never passing along the `ng-template` tag name. PR Close #52515

…ent projection This is a follow-up to the fix from angular#52414. It adds a diagnostic that will tell users when a control flow is preventing its direct descendants from being projected into a specific component slot.

…ent projection (#52726) This is a follow-up to the fix from #52414. It adds a diagnostic that will tell users when a control flow is preventing its direct descendants from being projected into a specific component slot. PR Close #52726

…ent projection This is a follow-up to the fix from angular#52414. It adds a diagnostic that will tell users when a control flow is preventing its direct descendants from being projected into a specific component slot.

…ent projection (#53190) This is a follow-up to the fix from #52414. It adds a diagnostic that will tell users when a control flow is preventing its direct descendants from being projected into a specific component slot. PR Close #53190

…ent projection (angular#53190) This is a follow-up to the fix from angular#52414. It adds a diagnostic that will tell users when a control flow is preventing its direct descendants from being projected into a specific component slot. PR Close angular#53190

angular-automatic-lock-bot · 2023-12-01T00:11:00Z

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

…ngular#52414) With the directive-based control flow users were able to conditionally project content using the `*` syntax. E.g. `<div *ngIf="expr" projectMe></div>` will be projected into `<ng-content select="[projectMe]"/>`, because the attributes and tag name from the `div` are copied to the template via the template creation instruction. With `@if` and `@for` that is not the case, because the conditional is placed *around* elements, rather than *on* them. The result is that content projection won't work in the same way if a user converts from `*ngIf` to `@if`. These changes aim to cover the most common case by doing the same copying when a control flow node has *one and only one* root element or template node. This approach comes with some caveats: 1. As soon as any other node is added to the root, the copying behavior won't work anymore. A diagnostic will be added to flag cases like this and to explain how to work around it. 2. If `preserveWhitespaces` is enabled, it's very likely that indentation will break this workaround, because it'll include an additional text node as the first child. We can work around it here, but in a discussion it was decided not to, because the user explicitly opted into preserving the whitespace and we would have to drop it from the generated code. The diagnostic mentioned point angular#1 will flag such cases to users. Fixes angular#52277. PR Close angular#52414

…template pipeline (angular#52414) Recreates the fix for content projection in control flow in the new template pipeline. I also had to make the following adjustments to the pipeline: 1. The `TemplateOp.tag` property was being used to generate the name of the template function, rather than the actual tag name being passed into `ɵɵtemplate`. Since the content projection fix requires the tag name to be passed in, I've introduced a new `functionNameSuffix` property instead. 2. `TemplateOp.block` was being used to determine whether to pass `TemplateOp.tag` into the `ɵɵtemplate` instruction. Now that we're always passing in the tag name after the refactor in point 1, we no longer need this flag. In addition to the refactors above, I also made some minor cleanups where I saw the opportunity to do so. PR Close angular#52414

…ngular#52414) With the directive-based control flow users were able to conditionally project content using the `*` syntax. E.g. `<div *ngIf="expr" projectMe></div>` will be projected into `<ng-content select="[projectMe]"/>`, because the attributes and tag name from the `div` are copied to the template via the template creation instruction. With `@if` and `@for` that is not the case, because the conditional is placed *around* elements, rather than *on* them. The result is that content projection won't work in the same way if a user converts from `*ngIf` to `@if`. These changes aim to cover the most common case by doing the same copying when a control flow node has *one and only one* root element or template node. This approach comes with some caveats: 1. As soon as any other node is added to the root, the copying behavior won't work anymore. A diagnostic will be added to flag cases like this and to explain how to work around it. 2. If `preserveWhitespaces` is enabled, it's very likely that indentation will break this workaround, because it'll include an additional text node as the first child. We can work around it here, but in a discussion it was decided not to, because the user explicitly opted into preserving the whitespace and we would have to drop it from the generated code. The diagnostic mentioned point angular#1 will flag such cases to users. Fixes angular#52277. PR Close angular#52414

…template pipeline (angular#52414) Recreates the fix for content projection in control flow in the new template pipeline. I also had to make the following adjustments to the pipeline: 1. The `TemplateOp.tag` property was being used to generate the name of the template function, rather than the actual tag name being passed into `ɵɵtemplate`. Since the content projection fix requires the tag name to be passed in, I've introduced a new `functionNameSuffix` property instead. 2. `TemplateOp.block` was being used to determine whether to pass `TemplateOp.tag` into the `ɵɵtemplate` instruction. Now that we're always passing in the tag name after the refactor in point 1, we no longer need this flag. In addition to the refactors above, I also made some minor cleanups where I saw the opportunity to do so. PR Close angular#52414

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: rc This PR is targeted for the next release-candidate labels Oct 27, 2023

crisbeto added this to the v17-final milestone Oct 27, 2023

crisbeto commented Oct 27, 2023

View reviewed changes

crisbeto force-pushed the control-flow-projection branch from 13d885e to 8bc2e23 Compare October 27, 2023 08:20

crisbeto requested review from dylhunn and pkozlowski-opensource October 27, 2023 08:51

crisbeto marked this pull request as ready for review October 27, 2023 08:51

crisbeto force-pushed the control-flow-projection branch from 8bc2e23 to 1ba5951 Compare October 27, 2023 09:12

crisbeto commented Oct 27, 2023

View reviewed changes

goldens/size-tracking/aio-payloads.json Outdated Show resolved Hide resolved

JeanMeche mentioned this pull request Oct 27, 2023

fix(core): replace assertion with more intentional error #52234

Closed

crisbeto force-pushed the control-flow-projection branch 2 times, most recently from 01ed981 to ad069fc Compare October 30, 2023 07:51

pkozlowski-opensource reviewed Oct 30, 2023

View reviewed changes

packages/core/test/acceptance/control_flow_for_spec.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the control-flow-projection branch from ad069fc to c8abae0 Compare October 30, 2023 10:20

pkozlowski-opensource approved these changes Oct 30, 2023

View reviewed changes

dylhunn approved these changes Oct 30, 2023

View reviewed changes

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 Oct 30, 2023

crisbeto added 2 commits October 31, 2023 21:48

crisbeto force-pushed the control-flow-projection branch from c8abae0 to d0b7cc3 Compare October 31, 2023 20:53

crisbeto added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 31, 2023

crisbeto mentioned this pull request Nov 5, 2023

fix(compiler): ng-template directive invoke twice at the root of control flow #52515

Closed

crisbeto mentioned this pull request Nov 9, 2023

Diagnostic to flag cases where control flow can break content projection #52726

Closed

iKrishnaSahu mentioned this pull request Nov 22, 2023

Control flow (@if etc) is not working when preserveWhitespaces is set to true in tsconfig #53114

Closed

crisbeto mentioned this pull request Nov 22, 2023

bug(MatFormField): mat-form-field and mat-icon with new @if @else condition angular/components#28172

Closed

1 task

crisbeto mentioned this pull request Nov 27, 2023

Diagnostic to flag cases where control flow can break content projection #53190

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project root node of @if and @for blocks #52414

Project root node of @if and @for blocks #52414

crisbeto commented Oct 27, 2023

crisbeto Oct 27, 2023

crisbeto commented Oct 30, 2023

pkozlowski-opensource left a comment

crisbeto commented Oct 30, 2023

pkozlowski-opensource left a comment

dylhunn left a comment

dylhunn Oct 30, 2023

crisbeto Oct 30, 2023

crisbeto commented Oct 31, 2023 •

edited

alxhub commented Oct 31, 2023

angular-automatic-lock-bot bot commented Dec 1, 2023

Project root node of @if and @for blocks #52414

Project root node of @if and @for blocks #52414

Conversation

crisbeto commented Oct 27, 2023

fix(compiler): project control flow root elements into correct slot

refactor(compiler): implement control flow content projection fix in template pipeline

crisbeto Oct 27, 2023

Choose a reason for hiding this comment

crisbeto commented Oct 30, 2023

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

crisbeto commented Oct 30, 2023

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

dylhunn Oct 30, 2023

Choose a reason for hiding this comment

crisbeto Oct 30, 2023

Choose a reason for hiding this comment

crisbeto commented Oct 31, 2023 • edited

alxhub commented Oct 31, 2023

angular-automatic-lock-bot bot commented Dec 1, 2023

crisbeto commented Oct 31, 2023 •

edited