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: weird behavior of ng-template migration #52518

Closed
zip-fa opened this issue Nov 5, 2023 · 3 comments
Closed

control flow: weird behavior of ng-template migration #52518

zip-fa opened this issue Nov 5, 2023 · 3 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

@zip-fa
Copy link

zip-fa commented Nov 5, 2023

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

core

Is this a regression?

No

Description

Hi. Migration of *ngFor block inside ng-template works weird.

Initial template:

<ng-container *ngIf="cond; else testTpl">
bla bla
</ng-container>

<ng-template #testTpl>
  <div class="test"
      *ngFor="let item of items"
    ></div>
</ng-template>

Result:

@if (cond) {
bla bla
} @else {
<div class="test"
  *ngFor="let item of items" <--- why this was not migrated
  ></div>
}

<ng-template #testTpl>
  @for (item of items; track item) { <--- while this was
  <div class="test"></div>
}
</ng-template>

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

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

Angular CLI: 17.0.0-rc.3
Node: 18.18.1
Package Manager: npm 9.8.1
OS: win32 x64

Angular: 17.0.0-rc.2
... animations, cdk, common, compiler, compiler-cli, core, forms
... localize, material, platform-browser
... platform-browser-dynamic, platform-server, router
... service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1700.0-rc.3
@angular-devkit/build-angular   17.0.0-rc.3
@angular-devkit/core            17.0.0-rc.3
@angular-devkit/schematics      17.0.0-rc.3
@angular/cli                    17.0.0-rc.3
@angular/ssr                    17.0.0-rc.3
@schematics/angular             17.0.0-rc.3
rxjs                            7.8.1
typescript                      5.2.2
zone.js                         0.14.0

Anything else?

No response

@zip-fa
Copy link
Author

zip-fa commented Nov 5, 2023

New observation: *ngIf gets ignored too.

@JeanMeche JeanMeche added area: migrations Issues related to `ng update` migrations core: control flow Issues related to the built-in control flow (@if, @for, @switch) labels Nov 5, 2023
@ngbot ngbot bot modified the milestone: needsTriage Nov 5, 2023
@jessicajaniuk jessicajaniuk self-assigned this Nov 6, 2023
@jessicajaniuk
Copy link
Contributor

This is expected, but not ideal. The migration processes top to bottom. So it sees the ngIf / else before the ngtemplate and processes that first, moving the content up from the ngTemplate. Since migrations are processed based on the original template contents, it doesn't know that the moved content from the ngTemplate has anything in it. It just knows the template contents later on has control flow to address. I think the proper solution is to migrate ngTemplates first before the rest of the file, so when this happens, the templates are already addressed. I'll take a look.

jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 7, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518, angular#52516, angular#52513
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 7, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518, angular#52516, angular#52513
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 7, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 7, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 7, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 7, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 7, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Nov 8, 2023
…bugs

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513
@atscott atscott closed this as completed in b9e2893 Nov 8, 2023
atscott pushed a commit that referenced this issue Nov 8, 2023
…bugs (#52592)

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: #52518
fixes: #52516
fixes: #52513

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

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513

PR Close angular#52592
@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 9, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
…bugs (angular#52592)

Rather than migrate all in one pass, this now migrates in a separate pass per control flow item plus one for templates at the end. This resolves issues with multiple control flow items on a single element as well as making sure ng-templates are fully migrated before being moved to new locations.

fixes: angular#52518
fixes: angular#52516
fixes: angular#52513

PR Close angular#52592
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

Successfully merging a pull request may close this issue.

3 participants