feat(core): Add schematic to migrate control flow syntax #52035

jessicajaniuk · 2023-10-04T21:20:49Z

This adds the migration to run to migrate to the block control flow syntax. It includes ngIf, ngFor, and ngSwitch.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Feature

Does this PR introduce a breaking change?

No

crisbeto

Looks great! I left a handful of nits, but they're not blockers.

packages/core/schematics/ng-generate/control-flow-migration/README.md

packages/core/schematics/ng-generate/control-flow-migration/util.ts

crisbeto · 2023-10-09T08:50:14Z

packages/core/schematics/ng-generate/control-flow-migration/util.ts

+    }
+    let offset = 0;
+
+    for (const el of visitor.elements) {


It might be simpler to have the visitor do the matching since it's already checking each element. We can also distribute the elements into different "buckets" based on which directive is applied. E.g.

class ElementCollector { ifNodes = new Set<Node>(); switchNodes = new Set<Node>(); forNode = new Set<Node>(); }

I started trying this, but found it actually complicates things a bit more. We still need the attribute info for later and tracking the offset becomes a bit noisier since we have to traverse each type separately.

packages/core/schematics/ng-generate/control-flow-migration/util.ts

crisbeto · 2023-10-09T09:09:50Z

packages/core/schematics/ng-generate/control-flow-migration/util.ts

+function getTmplStart(tmpl: string, elStart: number, templates: Template[]) {
+  let tmplStart = tmpl.slice(0, elStart);
+  for (const template of templates) {
+    if (template.count === 2 && template.el.sourceSpan.start.offset < elStart) {


I just realized that there's one case where this falls apart: a template that is referenced in a query. IMO we shouldn't handle it unless we get user reports about it, because it's highly unlikely that a template is used both as an else block and is queried for.

packages/core/schematics/ng-generate/control-flow-migration/util.ts

crisbeto · 2023-10-09T09:13:42Z

packages/core/schematics/test/control_flow_migration_spec.ts

+
+      writeFile('/comp.html', [
+        `<div>`,
+        `<span *ngIf="show; then thenBlock; else elseBlock">Ignored</span>`,


TIL that we even support the then input.

Yeah, I had a similar experience when writing this migration.

packages/core/schematics/test/control_flow_migration_spec.ts

AndrewKushnir

Just left a proposal to add test coverage for a few cases, but this is not critical (non-blocking), we can do it in a followup PR if needed.

AndrewKushnir · 2023-10-09T23:05:02Z

packages/core/schematics/test/control_flow_migration_spec.ts

+              `<p *ngSwitchCase="1">Option 1</p>` +
+              `<p *ngSwitchCase="2">Option 2</p>` +


I think it'd be great to cover the following cases (both are present in g3):

<ng-template [ngSwitchCase]="..." ...

<ng-template ngSwitchCase="..." ...

Similarly, other directives (ngIf, ngSwitch, ngSwitchDefault and ngFor) may have the same forms when added to <ng-template>.

crisbeto

LGTM

packages/core/schematics/ng-generate/control-flow-migration/util.ts

This adds the migration to run to migrate to the block control flow syntax. It includes ngIf, ngFor, and ngSwitch.

jessicajaniuk · 2023-10-10T18:25:46Z

Caretaker Note: This is safe to merge. The G3 failures are unrelated.

angular-automatic-lock-bot · 2023-12-10T00:11:07Z

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

This adds the migration to run to migrate to the block control flow syntax. It includes ngIf, ngFor, and ngSwitch. PR Close angular#52035

angular-robot bot added the detected: feature PR contains a feature commit label Oct 4, 2023

jessicajaniuk changed the title ~~feat(core): Add schematic to migrate control flow syntax~~ feat(core): Add schematic to migrate control flow syntax (WIP) Oct 4, 2023

jessicajaniuk added the area: core Issues related to the framework runtime label Oct 4, 2023

ngbot bot added this to the Backlog milestone Oct 4, 2023

jessicajaniuk added target: major This PR is targeted for the next major release state: WIP labels Oct 4, 2023

jessicajaniuk force-pushed the cf-migration branch 3 times, most recently from 8a0d7d7 to 8d6209c Compare October 5, 2023 15:52

jessicajaniuk modified the milestones: Backlog, v17-candidates Oct 5, 2023

jessicajaniuk force-pushed the cf-migration branch 4 times, most recently from fd31a93 to 7a6e286 Compare October 6, 2023 21:14

jessicajaniuk changed the title ~~feat(core): Add schematic to migrate control flow syntax (WIP)~~ feat(core): Add schematic to migrate control flow syntax Oct 6, 2023

jessicajaniuk marked this pull request as ready for review October 6, 2023 21:29

pullapprove bot requested a review from crisbeto October 6, 2023 21:29

jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Oct 6, 2023

crisbeto approved these changes Oct 9, 2023

View reviewed changes

JoostK reviewed Oct 9, 2023

View reviewed changes

packages/core/schematics/test/control_flow_migration_spec.ts Outdated Show resolved Hide resolved

jessicajaniuk force-pushed the cf-migration branch 2 times, most recently from 0755ef1 to 896437b Compare October 9, 2023 22:04

AndrewKushnir approved these changes Oct 9, 2023

View reviewed changes

crisbeto approved these changes Oct 10, 2023

View reviewed changes

packages/core/schematics/ng-generate/control-flow-migration/util.ts Show resolved Hide resolved

jessicajaniuk force-pushed the cf-migration branch from 896437b to c4b11aa Compare October 10, 2023 14:09

jessicajaniuk 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 action: merge The PR is ready for merge by the caretaker labels Oct 10, 2023

jessicajaniuk added the action: merge The PR is ready for merge by the caretaker label Oct 10, 2023

jessicajaniuk force-pushed the cf-migration branch 3 times, most recently from 5c5e01a to ec67f8a Compare October 10, 2023 17:10

feat(core): Add schematic to migrate control flow syntax

3851e8f

This adds the migration to run to migrate to the block control flow syntax. It includes ngIf, ngFor, and ngSwitch.

jessicajaniuk force-pushed the cf-migration branch from ec67f8a to 3851e8f Compare October 10, 2023 17:41

jessicajaniuk 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 10, 2023

angular-robot bot closed this in 50275e5 Oct 10, 2023

jessicajaniuk deleted the cf-migration branch November 9, 2023 20:34

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add schematic to migrate control flow syntax #52035

feat(core): Add schematic to migrate control flow syntax #52035

jessicajaniuk commented Oct 4, 2023

crisbeto left a comment

crisbeto Oct 9, 2023

jessicajaniuk Oct 9, 2023

crisbeto Oct 9, 2023

crisbeto Oct 9, 2023

jessicajaniuk Oct 9, 2023

AndrewKushnir left a comment

AndrewKushnir Oct 9, 2023

AndrewKushnir Oct 9, 2023

crisbeto left a comment

jessicajaniuk commented Oct 10, 2023

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

		`<p *ngSwitchCase="1">Option 1</p>` +
		`<p *ngSwitchCase="2">Option 2</p>` +

feat(core): Add schematic to migrate control flow syntax #52035

feat(core): Add schematic to migrate control flow syntax #52035

Conversation

jessicajaniuk commented Oct 4, 2023

PR Checklist

PR Type

Does this PR introduce a breaking change?

crisbeto left a comment

Choose a reason for hiding this comment

crisbeto Oct 9, 2023

Choose a reason for hiding this comment

jessicajaniuk Oct 9, 2023

Choose a reason for hiding this comment

crisbeto Oct 9, 2023

Choose a reason for hiding this comment

crisbeto Oct 9, 2023

Choose a reason for hiding this comment

jessicajaniuk Oct 9, 2023

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Oct 9, 2023

Choose a reason for hiding this comment

AndrewKushnir Oct 9, 2023

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

jessicajaniuk commented Oct 10, 2023

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