Function calls inside template are dangerous!
Tags: angular
Published: 2023-08-31 18:39:48

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](https://github.com/tomalaforge/angular-challenges/blob/main/apps/bug-cd/README.md) 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:

```ts
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:

```ts
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.

{% embed https://dev.to/this-is-angular/make-trackby-easy-to-use-28b5 %}

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:

```ts
@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](https://twitter.com/laforge_toma) or [Github](https://github.com/tomalaforge).Don't hesitate to reach out to me if you have any questions.