DEV Community

Cover image for Lessons learned from Angular's control flow migration script
Jared Robertson
Jared Robertson

Posted on

Lessons learned from Angular's control flow migration script

#angular

The old NgIf, NgFor, and NgSwitch directives are deprecated, and it's time to switch to the new control flow syntax @if, @for, and @switch. Angular's control flow migration script is really good, but it cannot correctly convert everything, so all changes must be manually checked.
My team has been hard at work with the conversion process. We have converted thousands of files, and only have about 150 files left. We have made many mistakes, and I hope that this article will help you avoid some of the pain we went through.

Do not run the script on your entire application

One huge pull request with all the changes is impossible to review, impossible to test, and you will ship regression bugs to your users.
I split our work up into tasks that each had fewer than 100 *ngIf statements in them. This worked as a starting point for a total time estimate. We have needed to split a few up into smaller tasks, and also combined some of them that were really small.

Avoid making non-script changes

A lot of our code is really old and was migrated from AngularJS, so there are plenty of clean-ups that we would like to do. As we started the conversion process, we would see all of these problems, and would ask the developer to make simple little fixes to clean things up. Do not fall into this trap. It is a very slippery slope.
Here are some of the "very small fixes" that turned into disasters.

Dangerous: Convert negated if-statements to if-else

In my opinion, the way to do an if-else statement with NgIf is difficult to read. Consequently, we would avoid using else statements, and would prefer doing something like this

<div *ngIf="loaded">
  ...
</div>
<div *ngIf="!loaded">Loading</div>
Enter fullscreen mode Exit fullscreen mode

When the script runs, it will turn into this

@if (loaded) {
  <div>
    ...
  </div>
}
@if (!loaded) {
  <div>Loading</div>
}
Enter fullscreen mode Exit fullscreen mode

It will be tempting to switch it to an else statement

@if (loaded) {
  <div>
    ...
  </div>
} @else {
  <div>Loading</div>
}
Enter fullscreen mode Exit fullscreen mode

DO NOT DO THIS!!! While it may be fine 99% of the time, there will likely be confusing code blocks that aren't actually negated if-statements. For example, the following code would create an infinite loading message if there was an error.

@if (loaded && !error) {
  <div>
    ...
  </div>
}
@if (!loaded && !error) {
  <div>Loading</div>
}
Enter fullscreen mode Exit fullscreen mode

This type of change is incredibly hard for reviewers and testers to catch. Just avoid it. You can fix it later.

Dangerous: Get rid of redundant div tags

A lot of our legacy code uses <div> tags that do nothing except hold the *ngIf directive (prefer <ng-container> instead).

After the conversion, the <div> will remain, look terrible, and you may want to just remove it.

<!-- Before conversion -->
<div *ngIf="condition">
  <div class="some-container">Contents</div>
</div>

<!-- After conversion -->
@if (condition) {
  <div><!-- Possibly unnecessary div -->
    <div class="some-container">Contents</div>
  </div>
}
Enter fullscreen mode Exit fullscreen mode

Again, DO NOT DO THIS!!! While it may be fine 99% of the time, there will likely be some piece of CSS somewhere that depends on that unnecessary div, and could completely mess up the page.
Again, doing this is hard to review, hard to test, and it's much safer to keep all of them in.

Not as dangerous: Replace let i = $index with just $index

At the start, we had a huge list of additional things to change while we were going through the migration process. The only one we have kept is replacing let i = $index with just $index and similar changes.

These are much safer because the compiler can catch a lot of potential problems they really stick out when reviewing them. We also found a bunch of places where we were declaring i but weren't actually using it anywhere.

Inline elements will have spacing added

This was the most common problem for us that we needed to fix. A lot of our legacy code handles plurality in a very i18n-unfriendly way:

You have {{ count }} item<span *ngIf="count !== 1">s</span>.
Enter fullscreen mode Exit fullscreen mode 
Output:
> You have 1 item.
> You have 3 items.
Enter fullscreen mode Exit fullscreen mode

After the migration, it will change to this

You have {{ count }} item
@if (count !== 1) {
  <span>s</span>
}
.
Enter fullscreen mode Exit fullscreen mode 
Output:
> You have 1 item .
> You have 3 item s .
Enter fullscreen mode Exit fullscreen mode

Because we are operating on inline elements, we get a bunch of extra spaces! These are easy to fix once you identify the problem.

{{
  count | i18nPlural : {
    "=1": "You have 1 item.",
    other: `You have ${count} items.`,
  }
}}
Enter fullscreen mode Exit fullscreen mode

To help identify these problems, I quickly wrote a simple node script using jsdom.

const jsdom = require("jsdom");
const { JSDOM } = jsdom;
const dom = new JSDOM(`<div>blah</div>`);

const ngIfElements = dom.window.document.querySelectorAll("[\\*ngIf]");
const allowedTagNames = ["DIV", "BUTTON", "MAT-SELECT", "MAT-TABLE"];
for (const element of ngIfElements) {
  // Print their type
  const tagName = element.tagName;
  if (!allowedTagNames.includes(tagName)) {
    // Print their test id
    if (element.hasAttribute("data-test-id")) {
      console.log(tagName, "-", element.getAttribute("data-test-id").trim());
    } else {
      console.log(tagName);
    }
    console.log(element.getAttribute("*ngIf").trim());
    console.log();
  }
}
Enter fullscreen mode Exit fullscreen mode

Replace <div>blah</div> with the pre-migrated HTML, it prints places that may be a problem.

*ngFor loops without a trackBy function

I am so happy that track is required in the new @for loop. We have seen major performance gains from this. Without it showing an error, it is too easy to forget.

Here is how the migration script automatically adds a track.

<!-- Before -->
<ng-container *ngFor="let item of items">
  <app-item [item]="item" />
</ng-container>

<!-- After -->
@for (item of items: track item) {
  <app-item [item]="item" />
}
Enter fullscreen mode Exit fullscreen mode

It works, but this is kind of the bare minimum, and you may get warnings in the browser saying it is inefficient. See the official documentation for specifics about track.
We decided to spend effort to optimize these. In most cases, we had a unique identifier we could use, like item.id:

@for (item of items: track item.id) {
  <app-item [item]="item" />
}
Enter fullscreen mode Exit fullscreen mode

Sometimes we weren't sure if there was a unique identifier, but knew the list would never change. In these cases we could use $index

@for (item of listThatWillNeverChange: track $index) {
  <app-item [item]="item" />
}
Enter fullscreen mode Exit fullscreen mode

And sometimes we still didn't know, so we would just use what the migration script gave us.

ng-template else blocks are removed

The migration script works perfectly fine here, but it is weird enough that I want to share it.

If our *ngIf statement has an else that is an <ng-template> block, the template will be removed and its contents will be put there instead.

<!-- Before -->
<ng-container *ngIf="loaded; else spinner">
  ...
</ng-container>
<ng-template #spinner>
  Loading...
</ng-template>

<!-- After -->
@if (loaded) {
  ...
} @else {
  Loading...
}
Enter fullscreen mode Exit fullscreen mode

This works perfectly most of the time, but it could be problematic if we were using that template in a lot of places. In our case, it never really added a lot of code, though we did have a few components where it added 10 or more copies of that same loading block.
It's also worth noting that if you have a comment above the <ng-template> block, it won't get copied over. So watch for that.

<!-- Before -->
<ng-container *ngIf="loaded; else spinner">
  ...
</ng-container>
<!-- Else block for main container -->
<ng-template #spinner>
  Loading...
</ng-template>

<!-- After -->
@if (loaded) {
  ...
} @else {
  Loading...
}
<!-- Else block for main container -->
Enter fullscreen mode Exit fullscreen mode

Random HTML in an *ngSwitch but outside an *ngCase

Yeah, some of our code is really bad and makes no sense. This worked before, but no longer works after the migration.

<span [ngSwitch]="count">
  Your cart is
  <ng-container *ngSwitchCase="0">
    empty.
  </ng-container>
  <ng-container *ngSwitchCase="1">
    full.
  </ng-container>
</span>
Enter fullscreen mode Exit fullscreen mode

The "Your cart is" text must be outside the switch for it to work.

Your cart is
<span [ngSwitch]="count">
  <ng-container *ngSwitchCase="0">
    empty.
  </ng-container>
  <ng-container *ngSwitchCase="1">
    full.
  </ng-container>
</span>
Enter fullscreen mode Exit fullscreen mode

Never formatted correctly

When you run the script, it asks if you want it to be formatted. This never worked for us, and we always had to format it ourselves after using prettier. The one problem we had to manually fix was that it would remove the imports for NgIf, NgFor, and NgSwitch, but it wouldn't remove the empty line it was on.

// Before
import { Component, inject } from "@angular/core";
import { NgIf, NgFor } from "@angular/common";
import { FormBuilder } from "@angular/forms";

// After
import { Component, inject } from "@angular/core";

import { FormBuilder } from "@angular/forms";
Enter fullscreen mode Exit fullscreen mode

Kind of weird, but not that big of a deal.

The end

Those were the problems we saw and how we dealt with them. I hope this helps!

Top comments (2)

Collapse
 
crdev profile image
C / •

Thanks for the article! And did you solve the issues manually?

Collapse
 
jared_robertson_d40011a40 profile image
Jared Robertson •

Yes, most of the problems we encountered we have had to solve manually on a case-by-case basis.

Some comments may only be visible to logged-in visitors. Sign in to view all comments.