DEV Community

Cover image for ngOnChanges best practice - always use SimpleChanges - always
Nick Raphael
Nick Raphael

Posted on • Updated on

ngOnChanges best practice - always use SimpleChanges - always

ngOnChanges(): "A lifecycle hook that is called when any data-bound property of a directive changes. Define an ngOnChanges() method to handle the changes."

We use this lifecycle hook to respond to changes to our @Input() variables. However, I'm seeing this method abused in the wild with, what I consider, an anti-pattern.

Let's start with an example...

import { Component, Input, OnChanges } from "@angular/core";

@Component({
  selector: "app-my-comp",
  templateUrl: "./my-comp.component.html"
})
export class MyComponent implements OnChanges {
  @Input() myFirstInputParameter: string;

  constructor() {}

  ngOnChanges() {
    this.doSomething(this.myFirstInputParameter);
  }

  private doSomething(input: string) {}
}

Enter fullscreen mode Exit fullscreen mode

This works fine. If the value of myInputParameter changes, our OnChanges fires and doSomething is called. Now let's add a second @Input...

import { Component, Input, OnChanges } from "@angular/core";

@Component({
  selector: "app-my-comp",
  templateUrl: "./my-comp.component.html"
})
export class MyComponent implements OnChanges {
  @Input() myFirstInputParameter: string;
  @Input() mySecondInputParameter: string;

  constructor() {}

  ngOnChanges() {
    this.doSomething(this.myFirstInputParameter);
  }

  private doSomething(input: string) {}
}

Enter fullscreen mode Exit fullscreen mode

This code now has a problem. OnChanges gets called twice - once for every @Input. What if the first time it's called is for mySecondInputParameter? Well, we'd be calling doSomething with 'undefined' since myFirstInputParameter hasn't been set yet. The way I typically see this handled is like this...

  OnChanges() {
    if (this.myFirstInputParameter) {
      this.doSomething(this.myFirstInputParameter);
    }
  }
Enter fullscreen mode Exit fullscreen mode

And this is the anti-pattern. Or worse, it's just plain wrong. Consider what happens if the first value to be set happens to be myFirstInputParameter. Well, our OnChanges fires, finds myFirstInputParameter is set and calls doSomething(). Then OnChanges fires again because mySecondInputParameter is set, finds that myFirstInputParameter is set and calls doSomething() again. This was not intended by the developer. If further @Input()'s are added, then doSomething() gets called even more. Does the angular developer really control the order that ngOnChanges() is called for each @Input? I'd say not. So writing code that assumes a particular order is bad. In fact, depending on your app, the order of calls to OnChanges may not be consistent and then your app may act inconsistently with race conditions.

The solution is to use SimpleChanges. And further to that - to ALWAYS use SimpleChanges. Lets see how SimpleChanges actually works. It's quite a simple change ;-)

https://angular.io/api/core/SimpleChanges

ngOnChanges(changes: SimpleChanges) {
    for (const propName in changes) {
      if (changes.hasOwnProperty(propName)) {
        switch (propName) {
          case 'myFirstInputParameter': {
            this.doSomething(change.currentValue)
          }
        }
      }
    }
  }
Enter fullscreen mode Exit fullscreen mode

This looks a bit verbose but it fixes our problem. ngOnChanges still gets called multiple times, but crucially the changes parameter tells us which @Input() caused the call. SimpleChanges also happens to provide the old value of the @Input() if there has been multiple changes to that value - pretty handy.

I propose that we should always take the SimpleChanges parameter in our ngOnChanges methods. Even if we only have a single @Input() on our component. I don't want code that assumes there will always be only one @Input(). That would be forcing the next developer to handle my @Input() if they want to add their own. It would be so easy to introduce a bug by adding another @Input() to a component and forgetting to check ngOnChanges() for the handling of the other @Input()'s. So take responsibility of your @Input() and embrace SimpleChanges.

Top comments (16)

Collapse
 
juanjoms profile image
Juan Miramontes

I have always used this approach to detect a specific change:

ngOnChanges(changes: SimpleChanges) {
  if (changes.myFirstInputParameter && changes.myFirstInputParameter.currentValue) {
    this.doSomething(this.myFirstInputParameter)
  }
}
Enter fullscreen mode Exit fullscreen mode

IMO it is simpler and more readable than the for/switch approach

Collapse
 
dennisf profile image
Dennis Frede • Edited

I currently go with the following. I think it's solid, and well readable:
defaultWidth and defaultHeight are some imported globally controlled variables.

  ngOnChanges(changes: SimpleChanges) {
    const {width, height, data} = changes;

    if (width) {
      const _width = width.currentValue ?? defaultWidth;
      // ...
    }

    if (height) {
      const _height = height.currentValue ?? defaultHeight;
      // ...
    }

    if (data) {
      const _data = data.currentValue ?? [];
      // ...
    }

  }
Enter fullscreen mode Exit fullscreen mode
Collapse
 
evolmk profile image
Miki (mike e) • Edited

need to add let change = changes[propName]; otherwise change.currentValue is undefined

ngOnChanges(changes: SimpleChanges) {
    for (const propName in changes) {
      if (changes.hasOwnProperty(propName)) {
        let change = changes[propName];
        switch (propName) {
          case 'pageSize': {
            console.log(`pageSize changed to:`, change.currentValue);
          }
        }
      }
    }
  }
Collapse
 
sebavicentea profile image
Seba Vicente

In this case, isn't it better to use a setter for each @Input instead of listen to ngOnChanges?

Collapse
 
nickraphael profile image
Nick Raphael

I think that comes down to personal preference. Personally, I find Input getters and setters to be too verbose. I like all inputs handled in the same place.

Collapse
 
hankchiutw profile image
Hank Chiu

I find Input getters and setters to be too verbose

checkout subjectize, which wraps the setter under the hood and you don't need a private variable.

Collapse
 
sproket profile image
sproket

WTF is 'change' in your example? I get cannot find name 'change'.

Collapse
 
mayanksamval profile image
mayanksamval

It's an oversight for sure! Use Changes[propName] instead

Collapse
 
kitkatsim profile image
Thiam Kiat Sim

this is a great way to illustrate how ngOnChanges works, thanks.
as pointed out by others, mistake in line:

this.doSomething(changes.myFirstInputParameter.currentValue)
Enter fullscreen mode Exit fullscreen mode
Collapse
 
ajayer profile image
Ajay
great
Enter fullscreen mode Exit fullscreen mode
Collapse
 
ajayer profile image
Ajay
thanks
Enter fullscreen mode Exit fullscreen mode
Collapse
 
miberg81_0 profile image
Michael Berger

I was practicing onChanges and ran into your post. Although I have only one input in my training app, I did fix it and added SimpleChanges to take care of the next developer, which happens to be me :)
I general life though from my observation there are few people who actually care about the next developer. Probably 2% from every 100 developers. It's sad, but everybody is obsessed to deliver smth. working, so people write some no so clean code. They get rewarded for such quick delivery, while nobody cares that the next developer will have a tough life...

Collapse
 
aydinmaz profile image
aydinmaz

Thanks man it was great ,I newly used ngOnChanges() , and by seeing your post I avoided future headaches !

Collapse
 
gadreash profile image
gadreash

Thanks Nick! Very helpful article.

Collapse
 
archieinet profile image
Archie

wt ?? it would be nice to put html in here..

Collapse
 
hbiori profile image
Hb Iori • Edited

I think that '' if (changes.hasOwnProperty(propName)) '' is useless
better to use '' if (changes[propName].currentValue) ''