Function calls inside template are dangerous!

The other day, one of my coworkers detected a strange behavior inside our application. When he added RouterLinkActive to a link, the application stopped rendering. However when the directive was removed, the application worked correctly.

Instead of immediately reading the solution, I created a challenge inside AngularChallenges for those who want to try resolving and understanding the source of the bug first. After that, you can come back to this article to compare your solution with mine and understand what went wrong.

To better understand the problem, below is a small reproduction of the issue:

interface MenuItem {
  path: string;
  name: string;
}

@Component({
  selector: 'app-nav',
  standalone: true,
  imports: [RouterLink, NgFor, RouterLinkActive],
  template: `
    <ng-container *ngFor="let menu of menus">
      <a
        [routerLink]="menu.path"
        [routerLinkActive]="isSelected"
          >
        {{ menu.name }}
      </a>
    </ng-container>
  `,
})
export class NavigationComponent {
  @Input() menus!: MenuItem[];
}

@Component({
  standalone: true,
  imports: [NavigationComponent, NgIf, AsyncPipe],
  template: `
    <ng-container *ngIf="info$ | async as info">
      <ng-container *ngIf="info !== null; else noInfo">
        <app-nav [menus]="getMenu(info)" />
      </ng-container>
    </ng-container>

    <ng-template #noInfo>
      <app-nav [menus]="getMenu('')" />
    </ng-template>
  `,
})
export class MainNavigationComponent {
  private fakeBackend = inject(FakeServiceService);

  readonly info$ = this.fakeBackend.getInfoFromBackend();

  getMenu(prop: string) {
    return [
      { path: '/foo', name: `Foo ${prop}` },
      { path: '/bar', name: `Bar ${prop}` },
    ];
  }
}

MainNavigationComponent will display NavigationComponent and pass a list of MenuItem as an argument depending on the return of an HTTP request. When our HTTP request returns, we call the getMenu function with an empty string if there is no info, or with info if it's not empty.

NavigationComponent will iterate over MenuItem and create a link for each item using RouterLink and RouterLinkActive for routing.

At first sight, this code seems correct, but applying RouterLinkActive to each link breaks the rendering with no errors in the console.

What could be happening? 🤯

To better understand the issue, let's break down RouterLinkActive with the code that is causing the infinite rendering loop:

import { Directive } from '@angular/core';

@Directive({
  selector: '[fake]',
  standalone: true,
})
export class FakeRouterLinkActiveDirective {
  constructor(private readonly cdr: ChangeDetectorRef) {
    queueMicrotask(() => {
      this.cdr.markForCheck();
    });
  }
}

Inside RouterLinkActive, we call this.cdr.markForCheck() to mark our component as dirty. However, we make this function call within a different micro task. Once our current macro task ends, Angular will trigger a new change detection cycle within the following micro task.

Having this information, can you spot the issue now ?

Since Angular runs a new change detection cycle, the framework rechecks every binding, causing new function calls. This means that the getMenu function inside our MainNavigationComponent will be called again, and it returns a new instance of MenuItems.

But that's not all.

NavigationComponent iterates over the array using the NgFor directive. As a new instance of MenuItem is passed as an Input to the component, NgFor recreates its list. NgFor destroys all DOM elements inside the list and recreates them. This causes the recreation of the RouterLinkActive instance, leading to another round of change detection, and this will be infinite.

We can avoid this by using the trackBy function inside the NgFor directive. This function tracks one property on the element, and checks if that property still exists within the new array. NgFor will only DESTROY or CREATE an element if the property no longer exists or did not exist previously. Adding the trackBy function in our case will correct the issue of infinite re-rendering.

If you are always forgetting the trackBy function, I invite you to read this article.

Make TrackBy Easy to Use!

thomas for This is Angular ・ Jul 21 '23

#angular

However, even if the trackBy function resolves this error, creating a new instance of MenuItem at each change detection cycle is bad practice.

One way of avoiding this would be to create a menuItem class property, but this would create imperative code and lead to spaghetti code.

The best way is to take a more declarative approach. Let's see how to refactor the code in a more declarative way:

@Component({
  standalone: true,
  imports: [NavigationComponent, AsyncPipe],
  template: ` <app-nav [menus]="(menus$ | async) ?? []" /> `,
})
export class MainNavigationComponent {
  private fakeBackend = inject(FakeServiceService);

  readonly menus$ = this.fakeBackend
    .getInfoFromBackend()
    .pipe(map((info) => this.getMenu(info ?? '')));

  getMenu(prop: string) {
    return [
      { path: '/foo', name: `Foo ${prop}` },
      { path: '/bar', name: `Bar ${prop}` },
    ];
  }
}

Our menus$ property is now defined in a single place, and will update when getInfoFromBackend returns. menu$ will not get recomputed at each change detection cycle and only a single instance will be created during the entire lifetime of MainNavigationComponent. The code looks simpler, doesn't it ?

You have probably heard that calling functions inside templates is a bad practice, and in most cases, it is. While you can call a function to access nested properties of an object, that should be one of the only exceptions. Try to avoid calling functions inside your template bindings, or be sure to really understand what you are doing and all the side effects that can be created by this function call. When attempting to mutate data through a function call, it should trigger a warning inside your head. Most of the time, there is a better declarative approach to your problem. Declarative programming is a different state of mind, but you should aim for it. Persist, and your code will become clearer and simpler, and both your coworkers and your future self will thank you for that.

I hope this article sheds some light on the consequences of calling functions within template bindings.

Notes: In the future, Angular with "signal" will reduce that risk. With "signal" being memoized, it will save you from recreating new instances. 🔥

You can find me on Twitter or Github.Don't hesitate to reach out to me if you have any questions.