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

Wrong 'this' in a service using Ivy #35167

Closed
waterplea opened this issue Feb 5, 2020 · 4 comments
Closed

Wrong 'this' in a service using Ivy #35167

waterplea opened this issue Feb 5, 2020 · 4 comments
Assignees
Labels
area: core Issues related to the framework runtime state: has PR type: bug/fix

Comments

@waterplea
Copy link
Contributor

waterplea commented Feb 5, 2020

🐞 bug report

Affected Package

@angular/core

Is this a regression?

Yes, the previous version in which this bug was not present was: 8

Description

I have a service abstraction over ngOnDestroy to unsubscribe from streams. Here's its source code, it's pretty simple:
@Injectable()
export class DestroyService extends Observable<void> implements OnDestroy {
    private readonly life$ = new Subject<void>();

    constructor() {
        super(subscriber => this.life$.subscribe(subscriber));
    }

    ngOnDestroy() {
        this.life$.next();
        this.life$.complete();
    }
}

I have it in my library of UI components that I compile with Angular 7/8. After I tried it with Angular 9 RC application (tried with 14 and with some old ones) — when a component (from my library) using that service gets destroyed I get an error Cannot read property 'next' of undefined. Setting breakpoints shown me that this inside this service is, for some reason, component instance that uses it, not the service instance.

🔬 Minimal Reproduction

I was unable to reproduce it in an isolated environment but I have no clue how this could have happened. My code is proprietary which I'm struggling to open source but it takes a while. I was hoping maybe somebody from Angular team who knows Ivy compiler and ngcc have any ideas how this could have theoretically happened.

🔥 Exception or Error

This inside ngOnDestroy of a service is component instance, not service instance

🌍 Your Environment

Angular Version:

Angular 9.0.0-rc.14!

Anything else relevant?
Sorry for such a vague description, the library is pretty straightforward Angular CLI library compiled with ng-packagr. It's just really huge and when I try to make a small library with relevant code — the issue does not reproduce. Without deep knowledge of how ngOnDestroy gets called I failed to debug it on my side.

@alxhub alxhub added area: core Issues related to the framework runtime type: bug/fix labels Feb 5, 2020
@ngbot ngbot bot modified the milestone: needsTriage Feb 5, 2020
@crisbeto
Copy link
Member

crisbeto commented Feb 5, 2020

How is the service being consumed? Are components extending it? I tried to reproduce it in a test like this, but it was behaving correctly. The logs is [Comp, InjectableWithHook]:

fit('', () => {
  const logs: any[] = [];

  @Injectable()
  class InjectableWithDestroyHook {
    ngOnDestroy() { logs.push(this); }
  }

  @Component({selector: 'comp', template: ''})
  class Comp extends InjectableWithDestroyHook {
  }

  @Component({template: '<comp></comp>', providers: [InjectableWithDestroyHook]})
  class App {
    constructor(foo: InjectableWithDestroyHook) {}
  }

  TestBed.configureTestingModule({declarations: [App, Comp]});
  const fixture = TestBed.createComponent(App);
  fixture.detectChanges();
  fixture.destroy();

  console.log(logs);
});

@AndrewKushnir AndrewKushnir added the needs reproduction This issue needs a reproduction in order for the team to investigate further label Feb 6, 2020
@waterplea
Copy link
Contributor Author

I've spent last two days trying to isolate it but I couldn't. Tried compiling smaller libraries with all the same versions but it wasn't reproducible :(

This is what I have:
https://ng-run.com/edit/Ukg0KPRiI0WMpdVaTWDc?open=app%2Fapp.module.ts&aot=true

There are 2 libraries in a workplace. One has services and a directive, second library has components that use that directive. When I compile both libraries and try to use those components in an Ivy application I get that error on destroy.

I'm going to try and debug it more, so far I'm clueless...

@waterplea
Copy link
Contributor Author

I got it reproduced, it happens in plain Ivy if you have two directives using the same service on the same element:

https://ng-run.com/edit/Ukg0KPRiI0WMpdVaTWDc?open=app%2Fapp.component.html&aot=true

Here you have a service listening to some events using that DestroyService as takeUntil and passing results to the directive. I've made two dummy directives like that, one listening to clicks and another listens to mousedowns, it doesn't really matter what they do. If you put one of them on an element it all works fine. But if you put both on the same element and that element disappears (click the button on the example) — then the error is shown. executeOnDestroys function from core.js gets wrong context in this line:
var context = view[destroyHooks[i]];

@kara kara added comp: ivy and removed needs reproduction This issue needs a reproduction in order for the team to investigate further labels Feb 6, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Feb 8, 2020
…times and don't call ngOnDestroy on multi provider

These changes fix the following regressions in Ivy:

1. When the same provider is resolved multiple times on the same node, the first invocation had the correct context, but all subsequent ones were incorrect because we were registering the hook multiple times under different indexes in `destroyHooks`.

2. Ivy was invoking the `ngOnDestroy` hook (with an incorrect context) on `multi` providers, whereas ViewEngine did not invoke it at all. I'm somewhat conflicted about this use case, because it feels like something that should be supported, but I decided to align it with the ViewEngine behavior. The reason why I aligned it is that with the current data structure there's no way to match up the destroy hook with the object that it belongs to, because the saved data looks something like this: `[5, ngOnDestroy, 10, ngOnDestroy]` and the indexes correspond to an array. Supporting this correctly would involve a bit more design work and the current behavior is breaking user apps.

Fixes angular#35167.
Fixes angular#35231.
crisbeto added a commit to crisbeto/angular that referenced this issue Feb 8, 2020
…times and don't call ngOnDestroy on multi provider

These changes fix the following regressions in Ivy:

1. When the same provider is resolved multiple times on the same node, the first invocation had the correct context, but all subsequent ones were incorrect because we were registering the hook multiple times under different indexes in `destroyHooks`.

2. Ivy was invoking the `ngOnDestroy` hook (with an incorrect context) on `multi` providers, whereas ViewEngine did not invoke it at all. I'm somewhat conflicted about this use case, because it feels like something that should be supported, but I decided to align it with the ViewEngine behavior. The reason why I aligned it is that with the current data structure there's no way to match up the destroy hook with the object that it belongs to, because the saved data looks something like this: `[5, ngOnDestroy, 10, ngOnDestroy]` and the indexes correspond to an array. Supporting this correctly would involve a bit more design work and the current behavior is breaking user apps.

Fixes angular#35167.
Fixes angular#35231.
@crisbeto crisbeto self-assigned this Feb 8, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Feb 10, 2020
…times and don't call ngOnDestroy on multi provider

These changes fix the following regressions in Ivy:

1. When the same provider is resolved multiple times on the same node, the first invocation had the correct context, but all subsequent ones were incorrect because we were registering the hook multiple times under different indexes in `destroyHooks`.

2. Ivy was invoking the `ngOnDestroy` hook (with an incorrect context) on `multi` providers, whereas ViewEngine did not invoke it at all. I'm somewhat conflicted about this use case, because it feels like something that should be supported, but I decided to align it with the ViewEngine behavior. The reason why I aligned it is that with the current data structure there's no way to match up the destroy hook with the object that it belongs to, because the saved data looks something like this: `[5, ngOnDestroy, 10, ngOnDestroy]` and the indexes correspond to an array. Supporting this correctly would involve a bit more design work and the current behavior is breaking user apps.

Fixes angular#35167.
Fixes angular#35231.
crisbeto added a commit to crisbeto/angular that referenced this issue Feb 14, 2020
…times

When the same provider is resolved multiple times on the same node, the first invocation had the correct context, but all subsequent ones were incorrect because we were registering the hook multiple times under different indexes in `destroyHooks`.

Fixes angular#35167.
@alxhub alxhub closed this as completed in 5fbfe69 Feb 19, 2020
alxhub pushed a commit that referenced this issue Feb 19, 2020
…times (#35249)

When the same provider is resolved multiple times on the same node, the first invocation had the correct context, but all subsequent ones were incorrect because we were registering the hook multiple times under different indexes in `destroyHooks`.

Fixes #35167.

PR Close #35249
@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 Mar 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime state: has PR type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants