loading...
Cover image for ngOnChanges best practice - always use SimpleChanges - always

ngOnChanges best practice - always use SimpleChanges - always

nickraphael profile image Nick Raphael Updated on ・2 min read

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) {}
}

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) {}
}

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);
    }
  }

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

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.

Discussion

pic
Editor guide
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)
  }
}

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

Collapse
evolmk profile image
Miki (mike e)

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
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
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 Author

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
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
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
aydinmaz profile image
aydinmaz

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

Collapse
djihadbengati profile image
Djihad BENGATI

Great ! Thank you :)