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

[ngUpgrade] onInit fires too early on downgraded components #18913

Closed
rkirov opened this issue Aug 28, 2017 · 12 comments · Fixed by #19394

Comments

@rkirov
Copy link
Contributor

commented Aug 28, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

The following component is downgraded and used in an AngularJS application:

export class Ng2ComponentBindings implements OnInit, OnChanges {
  @Input() foo: {a: number};

  ngOnInit() {
    console.log('ngInit:', this.foo);
  }

  ngOnChanges() {
    console.log('ngOnChange:', this.foo);
  }
}

ng1module.directive(
  'ng2ComponentBindings',  // lowerCamel when registering.
  downgradeComponent(
      {component: Ng2ComponentBindings, propagateDigest: true}));

it is used like this <ng2-component-bindings [foo]="$ctrl.foo"></ng2-component-bindings>

The order of events is:

  • ngOnInit fires
  • foo is set
  • ngOnChanges fires

Expected behavior

The order of events should be:

  • foo is set
  • ngOnChanges fires
  • ngOnInit fires

Because that is how a regular Angular component would behave.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

@rkirov, I tried to reproduce this here, but everything seems to work as expected. Maybe this behavior is affected by other stuff, such as structural directives.

What AngularJS/Angular versions are you using?
Can you try to reproduce it in the plnkr (or a sample repo)?

@rkirov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

Very odd indeed, the example looks very similar to the one I am looking at. Could it be AOT vs JIT? I will debug it a bit more.

@rkirov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

Do you have an NgUpgradeLite plunker template? I see the issue with an NgUpgradeLite.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

@gkalpak

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Oh look, another ngUpgradeLite template for StackBlitz

@rkirov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2017

Neat, thanks for the templates!

Here is the repro - https://stackblitz.com/edit/ngupgradelite-playground-2dputs

console shows:

onInit undefined
onChanges ngUpgradeLite
@gkalpak

This comment has been minimized.

Copy link
Member

commented Sep 23, 2017

This happens because we manually attach the componen't hostView to the ApplicationRef...

if (needsNgZone) {
this.appRef.attachView(this.componentRef.hostView);
}

...which apparently causes it to be change-detected too soon, before our AngularJS watcher updates the properties and calls the initial ngOnChanges()...

this.componentScope.$watch(() => this.inputChangeCount, this.wrapCallback(() => {
// Invoke `ngOnChanges()`
if (this.implementsOnChanges) {
const inputChanges = this.inputChanges;
this.inputChanges = {};
(<OnChanges>this.component).ngOnChanges(inputChanges !);
}

Should be an easy fix.

gkalpak added a commit to gkalpak/angular that referenced this issue Sep 26, 2017
@gkalpak gkalpak referenced this issue Sep 26, 2017
2 of 3 tasks complete
gkalpak added a commit to gkalpak/angular that referenced this issue Sep 26, 2017
gkalpak added a commit to gkalpak/angular that referenced this issue Oct 2, 2017
@alxhub alxhub closed this in #19394 Oct 2, 2017
alxhub added a commit that referenced this issue Oct 2, 2017
@VictorCoding

This comment has been minimized.

Copy link

commented Aug 31, 2018

Hello guys,

I'm not sure if this is related to this whole issue but one thing that is happening is that ngOnChanges is returning the following on the first run given

{
  currentValue: 9,
  firstChange: true,
  previousValue: 9,
}

as you can see, first of all previousValue is already set even though it should be undefined and second fo all, firstChange is true.

Reproduction Demo:

https://stackblitz.com/edit/ngupgradelite-playground-vttjsc

@gkalpak

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

The firstChange value is true, but we explicitly set previousValue equal to currentValue on first change. That is indeed not very intuitive (and it doesn't seem to be consistent with neither Angular nor AngularJS).

I think we want to be consistent with Angular here, which sets previousValue to undefined.

@VictorCoding, would you be interested in submitting a pull request with the fix? (I would be more than happy to provide help and guidance 😃)

@VictorCoding

This comment has been minimized.

Copy link

commented Sep 5, 2018

@gkalpak Thanks for the reply and confirmation of the situation. I'm down to give it a shot. I've looked around the code and I might have an idea where I can fix it but if anything comes up I'll reach out. Is this the best place to do that or should I open a new issue?

@gkalpak

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

That's awesome ❤️

Feel free to open a PR and we can use that to track and discuss the issue. If you want to discuss something before opening the PR, it's preferrable to open a new issue, since this is actually a separate problem.

@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Sep 14, 2019

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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.