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

Control-Flow migration does not support NgFor's ngForTemplate input #53068

Closed
pweyrich opened this issue Nov 20, 2023 · 2 comments
Closed

Control-Flow migration does not support NgFor's ngForTemplate input #53068

pweyrich opened this issue Nov 20, 2023 · 2 comments
Assignees
Labels
area: migrations Issues related to `ng update` migrations core: control flow Issues related to the built-in control flow (@if, @for, @switch)
Milestone

Comments

@pweyrich
Copy link

pweyrich commented Nov 20, 2023

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

No

Description

After running ng generate @angular/core:control-flow one of my components ended up being broken. The reason for this is that the migration is not accounting for NgFor's ngForTemplate input.

before the migration:

<ng-container *ngFor="let item of iterable; template: itemTemplate"></ng-container>
<ng-template #itemTemplate let-item>...</ng-template>

after the automated migration:

@for (item of iterable; track item) {}
<ng-template #itemTemplate let-item>...</ng-template>

As there doesn't seem to be an equivalent available with the new @for-block for this use case,
one would have to use the following workaround to get it running:

@for (item of iterable; track item) {
  <ng-container *ngTemplateOutlet="itemTemplate; context: {$implicit: item}"></ng-container>
}

<ng-template #itemTemplate let-item>...</ng-template>

Although this is easy to do manually, I think the automated migration should either properly handle this case or at least notify the user to take manual action! Otherwise, this issue might be hard to detect in bigger applications.

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

None.. Migration succeeded but my component was broken. It was not rendering the specific list anymore since its template contained an empty built-in @for-block.

Please provide the environment you discovered this bug in (run ng version)

Angular CLI: 17.0.2
Node: 18.18.2
Package Manager: npm 9.8.1
OS: darwin arm64

Angular: 17.0.3
... animations, common, compiler, compiler-cli, core, forms
... localize, platform-browser, platform-browser-dynamic, router
... service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1700.2
@angular-devkit/build-angular   17.0.2
@angular-devkit/core            17.0.2
@angular-devkit/schematics      17.0.2
@angular/cdk                    17.0.1
@angular/cli                    17.0.2
@angular/fire                   17.0.0-next.0
@angular/material               17.0.1
@schematics/angular             17.0.2
ng-packagr                      17.0.2
rxjs                            7.8.1
typescript                      5.2.2
zone.js                         0.14.2

Anything else?

While I know this is some rare use-case, I was wondering whether the above replacement is acceptable in terms of performance!? It will basically stamp out multiple template outlets which in turn render the specific template rather than rendering that template directly. If this indeed has performance drawbacks, should there maybe be an alternative solution for this use case with the new @for-block syntax?

@jessicajaniuk jessicajaniuk added the area: migrations Issues related to `ng update` migrations label Nov 21, 2023
@ngbot ngbot bot added this to the needsTriage milestone Nov 21, 2023
@jessicajaniuk jessicajaniuk added the core: control flow Issues related to the built-in control flow (@if, @for, @switch) label Nov 21, 2023
@jessicajaniuk jessicajaniuk self-assigned this Nov 21, 2023
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 21, 2023
This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068
@jessicajaniuk
Copy link
Contributor

Thanks for reporting this! You're correct in that it's a rare use case. I have a PR up that should address this use case and ensure it migrates properly. We'll discuss the performance implications with the team.

jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 21, 2023
This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 21, 2023
This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 27, 2023
This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068
pkozlowski-opensource pushed a commit that referenced this issue Nov 28, 2023
…53076)

This adds code to cover the rare use case of an ngFor with a template param.
fixes: #53068

PR Close #53076
Tweniee pushed a commit to Tweniee/angular that referenced this issue Nov 29, 2023
…ngular#53076)

This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068

PR Close angular#53076
tbondwilkinson pushed a commit to tbondwilkinson/angular that referenced this issue Dec 6, 2023
…ngular#53076)

This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068

PR Close angular#53076
@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 Dec 29, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
…ngular#53076)

This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068

PR Close angular#53076
rlmestre pushed a commit to rlmestre/angular that referenced this issue Jan 26, 2024
…ngular#53076)

This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068

PR Close angular#53076
amilamen pushed a commit to amilamen/angular that referenced this issue Jan 26, 2024
…ngular#53076)

This adds code to cover the rare use case of an ngFor with a template param.
fixes: angular#53068

PR Close angular#53076
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: migrations Issues related to `ng update` migrations core: control flow Issues related to the built-in control flow (@if, @for, @switch)
Projects
None yet
Development

No branches or pull requests

2 participants