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.

Sign up for GitHub

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

Jump to bottom

fix(ivy): wrong context passed to ngOnDestroy when resolved multiple times #35249

Closed
wants to merge 1 commit into from
Closed

fix(ivy): wrong context passed to ngOnDestroy when resolved multiple times #35249

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Feb 8, 2020 •
edited

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.

@crisbeto crisbeto marked this pull request as ready for review February 8, 2020 12:04
@crisbeto crisbeto added comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix labels Feb 8, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 8, 2020
Azbesciak
Azbesciak reviewed Feb 9, 2020
View reviewed changes
packages/core/src/render3/di_setup.ts Outdated
diPublicInInjector(
getOrCreateNodeInjectorForNode(
tNode as TElementNode | TContainerNode | TElementContainerNode, lView),
tView, token);
if (providerIsTypeProvider || isClassProvider(provider)) {
const prototype = ((provider as ClassProvider).useClass || provider).prototype;
const ngOnDestroy = prototype.ngOnDestroy;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What with useFactory for example when you provide some object which is not a class instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that one, but I'll bring it up. The previous approach didn't support useFactory either.

@Azbesciak
Copy link

Azbesciak commented Feb 9, 2020 •
edited

@crisbeto yeah, you are right. That would also explain why I always call ngOnDestroy from my root components manually also... because I noticed that this is sometimes called, sometimes not.

Thank you for your PR. Please, consider also my concerns in it.

@johannesnormannjensen
Copy link

johannesnormannjensen commented Feb 11, 2020 •
edited

This fix is really needed @pkozlowski-opensource

@Azbesciak
Copy link

@pkozlowski-opensource could you... :)

@crisbeto
fix(ivy): wrong context passed to ngOnDestroy when resolved multiple …
ef9d888 
…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.
@crisbeto crisbeto changed the title fix(ivy): wrong context passed to ngOnDestroy when resolved multiple times and don't call ngOnDestroy on multi provider fix(ivy): wrong context passed to ngOnDestroy when resolved multiple times Feb 14, 2020
kara
kara approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kara kara removed the request for review from pkozlowski-opensource February 18, 2020 23:38
@kara kara added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 18, 2020
@kara
Copy link
Contributor

kara commented Feb 18, 2020

presubmit

@kara kara removed the action: presubmit The PR is in need of a google3 presubmit label Feb 19, 2020
@alxhub alxhub closed this in 5fbfe69 Feb 19, 2020
alxhub pushed a commit that referenced this pull request Feb 19, 2020
@crisbeto @alxhub
fix(ivy): wrong context passed to ngOnDestroy when resolved multiple …
0671e54 
…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
@crisbeto crisbeto mentioned this pull request Feb 21, 2020
Closed
@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.
Reviewers

@Azbesciak Azbesciak Azbesciak left review comments

@kara kara kara approved these changes

@MarsiBarsi MarsiBarsi MarsiBarsi approved these changes

@waterplea waterplea waterplea approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

Wrong 'this' in a service using Ivy
7 participants
@crisbeto @Azbesciak @johannesnormannjensen @kara @MarsiBarsi @waterplea @googlebot