Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invoking ngOnChanges before ngOnInit is counter-intuitive. Why not change this? #7782

Closed
thorn0 opened this issue Mar 26, 2016 · 32 comments
Closed

Comments

@thorn0
Copy link
Contributor

thorn0 commented Mar 26, 2016

Steps to reproduce and a minimal demo of the problem

http://plnkr.co/edit/kIf8QgrFxtENzvr6V5TZ
Open the console to see the output there.

Current behavior

ngOnChanges of MyComponent is called before ngOnInit (docs).

Expected/desired behavior

I just have doubts about the idea of calling anything on a not fully initialized component. What if in ngOnChanges I want to use something that is initialized in ngOnInit ('That's where the heavy initialization logic belongs' as the docs say)? Why not change this order?

@ericmartinezr
Copy link
Contributor

It is by design, check the docs https://angular.io/docs/ts/latest/guide/lifecycle-hooks.html#!#hook-sequence

According to the docs

before ngOnInit and when a data-bound input property value changes.

Since you're passing inputs to your component is expected that gets fired.

@pkozlowski-opensource
Copy link
Member

Thnx @ericmartinezr !

@thorn0
Copy link
Contributor Author

thorn0 commented Apr 14, 2016

I actually didn't get any relevant answer. I read these docs, and I know that this behavior is by design. This issue is about the fact that this design is probably not optimal. Sorry if it's not very clear from my original message.

@zoechi
Copy link
Contributor

zoechi commented Apr 15, 2016

Calling ngOnInit() when bindings are not yet resolved would make ngOnInit() obsolete. You can set a flag to check if ngOnInit() has been called or whether ngOnChanges() is called the first time and then act accordingly, then you don't need ngOnInit()

@thorn0
Copy link
Contributor Author

thorn0 commented Apr 15, 2016

@zoechi So why not call ngOnInit after resolving bindings but before ngOnChanges? On the other hand, following your line of reasoning, does ngOnInit (even in its current position in the life cycle) really need to exist if we have ngOnChanges and, as you said, we can set flags to check for its first call?

However, I think all the logic that mostly deals with bindings (Lb) should be put to ngOnChanges. And all the other initialization logic that doesn't use the bindings much should go to ngOnInit (Li). This separation would look clear and logical. The question is what is more likely: for Lb to use the results of Li or vice versa? The answer to this question should determine the order in which the hooks are invoked.

@CaptainCodeman
Copy link

The ngOnInit / ngOnChanges sequence seem to be based on the case where the component inputs will not change and there will not actually be any ngOnChanges implemented. In this case it's OK to do the "heavy initialization" there as the docs suggest:

We turn to ngOnInit for two main reasons:

  • To perform complex initializations shortly after construction
  • To set up the component after Angular sets the input properties
    An ngOnInit often fetches data for the component as shown in the Tutorial and HTTP chapters.

We don't fetch data in a component constructor. Why? Because experienced developers agree that components should be cheap and safe to construct. We shouldn't worry that a new component will try to contact a remote server when created under test or before we decide to display it. Constructors should do no more than set the initial local variables to simple values.

When a component must start working soon after creation, we can count on Angular to call the ngOnInit method to jumpstart it. That's where the heavy initialization logic belongs.

Remember also that a directive's data-bound input properties are not set until after construction. That's a problem if we need to initialize the directive based on those properties. They'll have been set when our ngOninit runs.

But if you want your component to react to changes to it's inputs (surely all components should, isn't that the "contract" behind @Input() ?) then you have to start adding your own flags and handling what is then a less-useful call sequence because the first call telling you of the input values will be to a component that hasn't yet been initialized.

So you need to move the initialization code (or a call to it) to the ngOnChanges part anyway which makes ngOnInit a little irrelevant and a potential landmine to catch the unwary. I think most people would expect the sequence to be: constructor > init > changes (repeated as they happen). The current ordering feels unintuitive (it's a surprise and doesn't help people "fall into the pit of success").

If you want to respond to changes, you use ngOnChanges only.
If you want to ignore input changes, you use ngOnInit only.

I think you can argue for having ngOnInit OR ngOnChanges but that using both doesn't make much sense or add anything - so they could be simplified and combined (just have ngOnChanges?). I know it's probably too late for changes now, things have shipped in ng1 so we're probably stuck with it.

@thorn0
Copy link
Contributor Author

thorn0 commented Apr 17, 2016

@CaptainCodeman Actually, things haven't been shipped in ng1.

@pkozlowski-opensource Please consider reopening this issue. I believe it was closed by mistake.

@CaptainCodeman
Copy link

Actually, things haven't been shipped in ng1.

Sorry, I thought they had, the component and router are mentioned in the docs, they must be a version ahead?

https://docs.angularjs.org/guide/component
https://docs.angularjs.org/guide/component-router

I think 'issue closed' means "discussion over" - it's by design.

@thorn0
Copy link
Contributor Author

thorn0 commented Apr 17, 2016

The component lifecycle hooks are still under development in ng1. In particular, in the latest released version (1.5.3), $onChanges isn't called at all during the initialization. That was going to be included in 1.5.4, but this release has been cancelled for some reason. I believe it's still not too late to change the invocation order in both ng1 and ng2. At least, nobody is relying on the current order in ng1 now.

Okay, if it's by design, what are those design considerations that are behind the current hook sequence? Why is it a bad idea to change it? Or is it probably not so bad? It'd be great to get a substantive answer as these questions are going to be asked again and again due to the counter-intuitiveness of the current design.

@CaptainCodeman
Copy link

At least, nobody is relying on the current order in ng1 now.

I stand corrected, I thought the router and component stuff was already released for v1.x - so I agree, now is the time to change it if it needs changing.

Okay, if it's by design, what are those design considerations that are behind the current hook sequence? Why is it a bad idea to change it? Or is it probably not so bad? It'd be great to get a substantive answer as these questions are going to be asked again and again due to the counter-intuitiveness of the current design.

I was just pointing out that it's been discussed before and shut down as "by design", I wasn't suggesting I thought that it would be a bad idea to change it - I agree with you that it seems counter intuitive and unhelpful.

I don't know why some pieces seem open to more major re-design and even re-writes whereas other parts are not. Maybe differences in what people's view of "beta" means (e.g. is the API meant to be stable?) and what the implications of making various changes are.

@thorn0 thorn0 changed the title Is it okay that ngOnChanges is called before ngOnInit? Invoking ngOnChanges before ngOnInit is counter-intuitive. Why not change this? Apr 18, 2016
@pleerock
Copy link

pleerock commented Jul 3, 2016

+1 for rethinking its design

Inputs are coming before view and content init, and even ngOnInit, and if you are using setters on inputs to perform something that is depend of view components you can't do it on component initialization, but can do on next time inputs are changed. This creates a dummy code, like this:

export class DatePicker implements AfterViewInit {

    isInited = false;

    @Input()
    set data(data: string) {
        if (this.isInited)
           this.doSomethingViewRelated(data);
    }

   ngAfterViewInit() { // or ngOnInit as thorn0 described
       this.isInited = true;
       this.doSomethingViewRelated(this.data);
   }
}

@Bretto
Copy link

Bretto commented Sep 8, 2016

+1 for rethinking its design, I too think this is counter intuitive...

@fikkatra
Copy link

fikkatra commented Apr 13, 2017

If for some reason you want to ignore that first call to ngOnChanges (before ngOnInit was called), you can use the isFirstChange() function:

ngOnChanges(changes:{[key: string]: SimpleChange}) {
    if (!changes["myProp"].isFirstChange()) {
      // insert onchanges code
    }
  }

@AntiCZ
Copy link

AntiCZ commented May 29, 2017

@fikkatra I dont know. It is look like "Why do it simple if we can do it complicated"

@scottieslg
Copy link

+1 for rethinking this as well. It's very counter intuitive.

OnInit should be called when the initial values are set for any @inputs (hence the name Init), then OnChanges for any changes (hence the name Changes).

@just-paja
Copy link

+1 for rethinking for the same reason.

You can also consider renaming this lifecycle hook to increase intuitiveness. Here are some suggestions:

  • ngOnInitAfterFirstChangesWereMade
  • ngAfterRandomTimeout
  • ngOnInitFirstButIfngOnChangesIsDefinedCallItAfterThat with alias ngOnInitFirstButIfNgOnChangesIsDefinedCallItAfterThat

@vecernik
Copy link

vecernik commented Nov 8, 2017

+1 this is very confusing. can be worked around by accessing changes.item.firstChange

@mmalek06
Copy link

+1 for rethinking it. It's confusing as hell - makes me trip over it each time.

@lscarneiro
Copy link

For one whose have a component with a @Input that must be call the next() from a Subject that have a subscribe() within ngOnInit() block and don't want to put the inner subscribe() login in the ngOnChanges block, I suggest to cal the subject.next() within a setTimeout(...,0) block like this:

private $onChange = new Subject();
ngOnInit() {
     ...
     this.$onChange.subscribe(response => {
          //some important code that should occur event in the first @Input set event
     });
     ...
}
ngOnChanges(changes: SimpleChanges): void {
     ...
     if (SomeVerification) {
          ...
          setTimeout(() => {
               this.$onChange.next(this.stateCode);
          }, 0);
          ...
     }
     ...
}

This solved my problem

@hidegh
Copy link

hidegh commented Jun 14, 2018

@lscarneiro nice, would probably use ReplaySubject to catch situations where the ngOnChanges occure before ngOnInit - see: https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/subjects/replaysubject.md

@nukec
Copy link

nukec commented Aug 3, 2018

+1 here for improving this. no necessity to run on changes on init, it's self explanatory.

@hidegh
Copy link

hidegh commented Aug 3, 2018

or a +1 to add a ngOnAfterConstruction call which should always be in prior of ngOnChanges and ngOnInit - and thus no breaking change.

@BenMakesGames
Copy link

just wanted to cast my vote here that this is very counterintuitive. I just now found this thread while trying to figure out what was going on in my code; it SEEMED like ngOnInit was being called after ngOnChanges... "but that's crazy", I thought. "I must be doing something else wrong; init obviously happens before changes". apparently I was wrong...

@mlc-mlapis
Copy link
Contributor

mlc-mlapis commented Sep 10, 2018

... there is certainly a reason why ngOnChanges is called first in the actual code. Has anybody gone through the source code to recapitulate the actual logic?

@hidegh
Copy link

hidegh commented Sep 10, 2018

@mlc-mlapis the Init is something everyone is expecting as the next event. the issue is that init can be the next event, but once you bind to an input, then ngOnChanges is fired before. it's documented, yet this does not mean it has a good developer experience. I myself really miss an event that occures right after ctor, because in ctors you can't use async/await. and with ngOnChanges / ngOnInit occuring in a deterministic, predicable but non static order there's no way to do some "magic" without adding extra boilerplate code. and if a new tool forces you to write BS (boilerplate) then we should really ask: how modern that tool is.

I really like Angular, but whenever I had some issues, I always asked: how solves that Rob Eisenberg?

Believe or not:

  • angular works well with template driven binding
  • even 3rd party validation can be bound to UI (although I had to do 2 weeks of R&D to get the final code that allowed me to reuse template driven forms with custom validaiton logic from the main component)
  • yet angular does some things by their way, ignoring what developers need (forcing model driven way, not adding better - less verbose - way to display errors, etc...

@mlc-mlapis
Copy link
Contributor

@hidegh ... ah, no, no, I didn't mean that the suggestion is wrong itself ... I just wanted to say that exact formulation, why the actual logic and naming is as it is, can help us to suggest even better names. 😄

@stweedie
Copy link

Has there been a clarification as to why this is the expected behavior? I would have expected the following to work:

@Component()
export class SomeFormComponent {
  @Input() someObject: object;
  @Input() parentForm: FormGroup;

  ngOnInit() {
    // add controls to parent form
    parentForm.registerControl('name', new FormControl(this.someObject.name, [Validators.required]));
  }

  ngOnChanges(changes) {
    // someObject was changed to different object, say in case of parent - detail
    const someObjectChanged = changes.someObject;
    if (someObjectChanged) {
      this.parentForm.get('name').setValue(someObjectChanged.currentValue.name);
    }
  }
}

yet it doesn't because ngOnChanges will fire before ngOnInit.

I realize there are workarounds. In this case, for instance, I would have to check 'isFirstChange' in ngOnChanges to make sure that the form exists

@hgoebl
Copy link

hgoebl commented Jul 8, 2019

It's a bit strange that isFirstChange is not a member of SimpleChanges.

I'm using this to detect if it's the first change. Do you think it's correct?

ngOnChanges(changes: SimpleChanges): void {
  const isInitialization = Object.keys(changes).some(key => changes[key].isFirstChange());
  if (isInitialization) {
    // ... maybe call some initialization if you don't implement `OnInit`
    return;
  }
  // ... typical code for handling "real" changes comes here
}

@hidegh
Copy link

hidegh commented Jul 8, 2019

@hgoebl according this it is: https://angular.io/api/core/SimpleChange but still, the ngOnInit is not always the 1st call, which is all but unnatural and needs some boilerplate code.

@stweedie
Copy link

stweedie commented Jul 8, 2019

ngOnChanges gets a SimpleChanges object, which is a collection of SimpleChange objects. SimpleChanges does not have a way to determine if it's the first change or not.

More to the point though, there still has been no explanation why the order of lifecycle hooks is the way it is, other than "it's by design" (which is not an explanation)

@hgoebl
Copy link

hgoebl commented Jul 8, 2019

I’ve seen many components not implementing OnChanges, but only OnInit. For them, it’s comfortable that at the time of ngOnInit the first filling of the input values is already done.
But it always looks a bit broken when input changes are not handled.

I wouldn’t spend more time discussing the design decision. Seems like this won’t change…

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests