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.

Sign up for GitHub

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

Jump to bottom

Control Flow Migration does not work correctly on template #53779

Closed
yharaskrik opened this issue Jan 3, 2024 · 6 comments
Closed

Control Flow Migration does not work correctly on template #53779

yharaskrik opened this issue Jan 3, 2024 · 6 comments
Assignees
@jessicajaniuk
Labels
area: migrations Issues related to `ng update` migrations core: control flow Issues related to the built-in control flow (@if, @for, @switch)
Milestone
needsTriage

Comments

@yharaskrik
Copy link
Contributor

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

core

Is this a regression?

Yes

Description

Running control flow migration on the following template does not work correctly.

Original Template:

<ng-container *ngIf="generic; else specific">
    <ng-container [ngSwitch]="dueWhen">
        <ng-container *ngSwitchCase="due.NOW">
            <div class="alert alert-danger generic" [template]="tooltip" trTemplateTooltip position="centre-end">
                Required Information
            </div>
        </ng-container>
        <ng-container *ngSwitchCase="due.SOON">
            <div class="alert alert-warning generic" [template]="tooltip" trTemplateTooltip position="centre-end">
                Information Required Soon
            </div>
        </ng-container>
        <ng-container *ngSwitchDefault></ng-container>
    </ng-container>
</ng-container>
<ng-template #specific>
    <ng-container [ngSwitch]="dueWhen">
        <ng-container *ngSwitchCase="due.NOW">
            <div
                class="alert alert-danger"
                matTooltip="Stripe requires this information immediately to verify your account.">
                Required Now
            </div>
        </ng-container>
        <ng-container *ngSwitchCase="due.SOON">
            <div class="alert alert-warning" matTooltip="Stripe may require this information in the future.">
                Required Soon
            </div>
        </ng-container>
        <ng-container *ngSwitchDefault>
            <div class="trellis-p" [ngClass]="{ 'text-muted': !value && !overrideValue }">
                {{ value && !overrideValue ? value : altValue }}
            </div>
        </ng-container>
    </ng-container>
</ng-template>
<ng-template #tooltip>
    <tr-stripe-banking-missing-info [account]="account"></tr-stripe-banking-missing-info>
</ng-template>

Migrates to:

@if (generic) {
    @switch (dueWhen) {
        @case (due.NOW) {
            <div class="alert alert-danger generic" [template]="tooltip" trTemplateTooltip position="centre-end">
                Required Information
            </div>
        }
        @case (due.SOON) {
            <div class="alert alert-warning generic" [template]="tooltip" trTemplateTooltip position="centre-end">
                Information Required Soon
            </div>
        }
        @default {}
    }
} @else {
    @switch (dueWhen) {
        @case (due.NOW) {
            ">
            <div
                class="alert alert-danger"
                matTooltip="Stripe requires this information immediately to verify your account.">
                Required Now
            </div>
        }
        r>
        @case (due.SOON) {
            ">
            <div class="alert alert-warning" matTooltip="Stripe may require this information in the future.">
                Required Soon
            </div>
        }
        r>
        @default {
            t>
            <div class="trellis-p" [ngClass]="{ 'text-muted': !value && !overrideValue }">
                {{ value && !overrideValue ? value : altValue }}
            </div>
        }
        r>
    }
}

<ng-template #tooltip>
    <tr-stripe-banking-missing-info [account]="account"></tr-stripe-banking-missing-info>
</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)

No response

Anything else?

No response
@pkozlowski-opensource pkozlowski-opensource added area: migrations Issues related to `ng update` migrations core: control flow Issues related to the built-in control flow (@if, @for, @switch) labels Jan 3, 2024
@ngbot ngbot bot added this to the needsTriage milestone Jan 3, 2024
@OBe95
Copy link

OBe95 commented Jan 5, 2024

I was able to reproduce with the cli with @angular/core:control-flow

However, running the control-flow schematic from the main branch produces a correct migration, so I guess it's a matter of bumping @angular/core in the cli

I see @jessicajaniuk is the main contributor to the migration schematic, maybe you can confirm ?

@jessicajaniuk jessicajaniuk self-assigned this Jan 8, 2024
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this issue Jan 8, 2024
@thePunderWoman
fix(migrations): Fix empty switch case offset bug in cf migration
3afe86d 
This addresses the offset issue caused when a switch case was empty with no spaces or children being affected by the markers that were added, but not accounted for in offset. The markers are not needed for empty content and can be safely removed in this case.

fixes: angular#53779
@jessicajaniuk jessicajaniuk mentioned this issue Jan 8, 2024
Closed
6 tasks
@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Jan 8, 2024 •
edited

I've been able to replicate this behavior and found the issue. The empty <ng-container *ngSwitchDefault></ng-container> was being migrated in a way that was throwing the offset off by 2 characters. This would only happen with empty ng-containers being converted to the new control flow where the control flow was on that container. Should be fixed after #53839 lands.

@OBe95
Copy link

OBe95 commented Jan 8, 2024

@jessicajaniuk I am wondering if it was straightforward to reproduce

And if you would have an idea why I couldn't reproduce locally with the main branch (I started with a similar unit test to the one in your PR and it went through successfully then with the schematic same successfully migrated)

@jessicajaniuk
Copy link
Contributor

@OBe95 Without seeing your setup, I don't know. Even a single space or a line break in the ngSwitchDefault would make the test pass. So it could just be that.

@OBe95
Copy link

OBe95 commented Jan 8, 2024

I thought maybe there is a known pitfall that I missed
I don't think I had an extra whitespace, I'll retry and dive more into it
Thank you 🙏

atscott pushed a commit that referenced this issue Jan 9, 2024
@thePunderWoman @atscott
fix(migrations): Fix empty switch case offset bug in cf migration (#5…
e92c86b 
…3839)

This addresses the offset issue caused when a switch case was empty with no spaces or children being affected by the markers that were added, but not accounted for in offset. The markers are not needed for empty content and can be safely removed in this case.

fixes: #53779

PR Close #53839
@atscott atscott closed this as completed in d0b95d5 Jan 9, 2024
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
@thePunderWoman @ChellappanRajan
fix(migrations): Fix empty switch case offset bug in cf migration (an…
733eee7 
…gular#53839)

This addresses the offset issue caused when a switch case was empty with no spaces or children being affected by the markers that were added, but not accounted for in offset. The markers are not needed for empty content and can be safely removed in this case.

fixes: angular#53779

PR Close angular#53839
rlmestre pushed a commit to rlmestre/angular that referenced this issue Jan 26, 2024
@thePunderWoman @rlmestre
fix(migrations): Fix empty switch case offset bug in cf migration (an…
835af50 
…gular#53839)

This addresses the offset issue caused when a switch case was empty with no spaces or children being affected by the markers that were added, but not accounted for in offset. The markers are not needed for empty content and can be safely removed in this case.

fixes: angular#53779

PR Close angular#53839
danieljancar pushed a commit to danieljancar/angular that referenced this issue Jan 26, 2024
@thePunderWoman @danieljancar
fix(migrations): Fix empty switch case offset bug in cf migration (an…
aa090ed 
…gular#53839)

This addresses the offset issue caused when a switch case was empty with no spaces or children being affected by the markers that were added, but not accounted for in offset. The markers are not needed for empty content and can be safely removed in this case.

fixes: angular#53779

PR Close angular#53839
amilamen pushed a commit to amilamen/angular that referenced this issue Jan 26, 2024
@thePunderWoman @amilamen
fix(migrations): Fix empty switch case offset bug in cf migration (an…
abe5b62 
…gular#53839)

This addresses the offset issue caused when a switch case was empty with no spaces or children being affected by the markers that were added, but not accounted for in offset. The markers are not needed for empty content and can be safely removed in this case.

fixes: angular#53779

PR Close angular#53839
@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 Feb 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jessicajaniuk jessicajaniuk

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
Milestone
needsTriage
Development

Successfully merging a pull request may close this issue.

fix(migrations): Fix empty switch case offset bug in cf migration jessicajaniuk/angular
4 participants
@pkozlowski-opensource @yharaskrik @OBe95 @jessicajaniuk