8000 fix(ivy): component destroy hook called twice when configured as provider by crisbeto · Pull Request #28470 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

fix(ivy): component destroy hook called twice when configured as provider #28470

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 Git 10000 Hub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

fix(ivy): component destroy hook called twice when configured as provider #28470

wants to merge 3 commits into from

Conversation

crisbeto
Copy link
Member

Fixes the ngOnDestroy hook on a component or directive being called twice, if the type is also registered as a provider.

This PR resolves FW-1010.

@crisbeto crisbeto self-assigned this Jan 31, 2019
@crisbeto crisbeto requested a review from a team as a code owner January 31, 2019 09:40
@crisbeto crisbeto changed the title wip(ivy): component destroy hook called twice when configured as provider fix(ivy): component destroy hook called twice when configured as provider Jan 31, 2019
@crisbeto crisbeto removed their assignment Jan 31, 2019
@crisbeto crisbeto added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy and removed state: WIP labels Jan 31, 2019
@ngbot ngbot bot modified the milestone: needsTriage Jan 31, 2019
kara
kara suggested changes Jan 31, 2019
View reviewed changes
@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 31, 2019
@crisbeto crisbeto removed their assignment Jan 31, 2019
@crisbeto crisbeto removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 31, 2019
@crisbeto
Copy link
Member Author
crisbeto commented Jan 31, 2019 •
edited
Loading

Edit: I had to revert the firstTemplatePass check, because it ended up breaking some tests for lazy-loaded modules.

The feedback has been addressed.

@crisbeto crisbeto force-pushed the FW-1010/provider-on-destroy-twice branch from ed66585 to c677d78 Compare January 31, 2019 18:14
@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 31, 2019
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 31, 2019
@crisbeto crisbeto removed their assignment Jan 31, 2019
@mhevery mhevery self-assigned this Jan 31, 2019
@crisbeto crisbeto added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 18, 2019
filipesilva
filipesilva approved these changes Feb 20, 2019
View reviewed changes
Copy link
Contributor
@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

The changes in tools/material-ci/angular_material_test_blocklist.js LGTM.

@crisbeto crisbeto force-pushed the FW-1010/provider-on-destroy-twice branch 3 times, most recently from 483a799 to 3acd4f9 Compare February 20, 2019 16:16
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 20, 2019
@crisbeto crisbeto removed their assignment Feb 20, 2019
@crisbeto
Copy link
Member Author

@kara I've moved the unit tests to TestBed and I've updated the blocklist, however switching to TestBed revealed a discrepancy between Ivy and ViewEngine in these two tests:

  • should not inherit ngOnDestroy hooks on providers
  • In Ivy the destroy hook of useClass provider is invoked

I've marked them as "only in Ivy", but I think that it's up for discussion how we should handle them.

@crisbeto
fix(ivy): component destroy hook called twice when configured as prov…
bfe690b 
…ider

Fixes the `ngOnDestroy` hook on a component or directive being called twice, if the type is also registered as a provider.

This PR resolves FW-1010.
@crisbeto
fixup! fix(ivy): component destroy hook called twice when configured …
8ee55fe 
…as provider
@crisbeto crisbeto force-pushed the FW-1010/provider-on-destroy-twice branch from 3acd4f9 to 252180c Compare February 21, 2019 13:51
@crisbeto
Copy link
Member Author

I've updated it based on our discussion @kara.

@crisbeto crisbeto force-pushed the FW-1010/provider-on-destroy-twice branch from 252180c to a581753 Compare February 21, 2019 14:51
@crisbeto crisbeto requested a review from a team as a code owner February 21, 2019 14:51
kara
kara approved these changes Feb 21, 2019
View reviewed changes
Copy link
Contribu 8000 tor
@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
Copy link
Contributor
kara commented Feb 21, 2019

presubmit

@kara kara added action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker labels Feb 21, 2019
@kara
Copy link
Contributor
kara commented Feb 21, 2019

merge assistance: global approval

@IgorMinar IgorMinar added the target: major This PR is targeted for the next major release label Feb 21, 2019
@IgorMinar IgorMinar closed this in e1aaa7e Feb 21, 2019
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kara kara kara approved these changes

@filipesilva filipesilva filipesilva approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.
6 participants
@crisbeto @kara @filipesilva @mhevery @IgorMinar @googlebot
0