Skip to content

fix(ivy): destroy injector when module is destroyed#27793

Closed
crisbeto wants to merge 1 commit intoangular:masterfrom
crisbeto:FW-739/ng-module-injector-destroy
Closed

fix(ivy): destroy injector when module is destroyed#27793
crisbeto wants to merge 1 commit intoangular:masterfrom
crisbeto:FW-739/ng-module-injector-destroy

Conversation

@crisbeto
Copy link
Copy Markdown
Member

@crisbeto crisbeto commented Dec 21, 2018

Destroys the module's injector when an NgModule is destroyed which in turn calls the ngOnDestroy methods on the instantiated providers.

This PR resolves FW-739.

@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Dec 21, 2018

You can preview e47e586 at https://pr27793-e47e586.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from e47e586 to 27b8357 Compare December 21, 2018 11:03
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Dec 21, 2018

You can preview 27b8357 at https://pr27793-27b8357.ngbuilds.io/.

@crisbeto crisbeto self-assigned this Dec 21, 2018
@crisbeto crisbeto changed the title fix(ivy): destroy injector when module is destroyed wip(ivy): destroy injector when module is destroyed Dec 21, 2018
mhevery
mhevery approved these changes Dec 21, 2018
View reviewed changes
Comment thread packages/core/src/di/r3_injector.ts Outdated
Copy link
Copy Markdown
Contributor

@mhevery mhevery Dec 21, 2018

Choose a reason for hiding this comment

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

Why turning this into getter?

Copy link
Copy Markdown
Member Author

@crisbeto crisbeto Dec 21, 2018

Choose a reason for hiding this comment

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

I want to prevent people from being able to set it from the outside. It could also be turned into a isDestroyed method which'll generate slightly less ES5 code.

@crisbeto
Copy link
Copy Markdown
Member Author

crisbeto commented Dec 21, 2018

Note to self: check whether these changes fix the FW-848 issues as well.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 27b8357 to 5c60d61 Compare December 23, 2018 06:37
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Dec 23, 2018

You can preview 5c60d61 at https://pr27793-5c60d61.ngbuilds.io/.

@mhevery mhevery self-assigned this Jan 15, 2019
@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 5c60d61 to 6b2e131 Compare January 16, 2019 06:23
@crisbeto crisbeto requested review from a team January 16, 2019 06:23
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 6b2e131 at https://pr27793-6b2e131.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 6b2e131 to 93eac72 Compare January 16, 2019 06:39
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 93eac72 at https://pr27793-93eac72.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 93eac72 to e2487fc Compare January 16, 2019 07:45
@crisbeto crisbeto requested a review from a team January 16, 2019 07:45
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview e2487fc at https://pr27793-e2487fc.ngbuilds.io/.

petebacondarwin
petebacondarwin reviewed Jan 16, 2019
View reviewed changes
Comment thread packages/elements/test/slots_spec.ts Outdated
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin Jan 16, 2019

Choose a reason for hiding this comment

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

Why this change? Does calling testContainer.remove() not remove it from its parent? That would seem like an obvious thing for such a method to do...

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch 2 times, most recently from 859f7ad to 7950c8c Compare January 16, 2019 08:24
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 7950c8c at https://pr27793-7950c8c.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 7950c8c to 82aca8c Compare January 16, 2019 08:32
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 82aca8c at https://pr27793-82aca8c.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 82aca8c to 684ed75 Compare January 16, 2019 08:56
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 684ed75 at https://pr27793-684ed75.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 684ed75 to 59be577 Compare January 16, 2019 09:12
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 59be577 at https://pr27793-59be577.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 59be577 to 139c3b2 Compare January 16, 2019 19:07
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 139c3b2 at https://pr27793-139c3b2.ngbuilds.io/.

@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 139c3b2 to 434e8f8 Compare January 16, 2019 19:18
kara
kara suggested changes Jan 16, 2019
View reviewed changes
Comment thread packages/core/src/render3/node_manipulation.ts Outdated
Copy link
Copy Markdown
Contributor

@kara kara Jan 16, 2019

Choose a reason for hiding this comment

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

Hmm this doesn't look quite right since TViews are shared. If you remove one view instance, wouldn't this mean that when other view instances are removed, the cleanup fns would be gone? Let's add a test for this ?

@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 434e8f8 at https://pr27793-434e8f8.ngbuilds.io/.

@crisbeto
fix(ivy): destroy injector when module is destroyed
1eebc40 
Destroys the module's injector when an `NgModule` is destroyed which in turn calls the `ngOnDestroy` methods on the instantiated providers.

This PR resolves FW-739.
@crisbeto crisbeto force-pushed the FW-739/ng-module-injector-destroy branch from 434e8f8 to 1eebc40 Compare January 16, 2019 19:36
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Jan 16, 2019

You can preview 1eebc40 at https://pr27793-1eebc40.ngbuilds.io/.

@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 16, 2019
@crisbeto crisbeto removed their assignment Jan 16, 2019
@crisbeto crisbeto changed the title wip(ivy): destroy injector when module is destroyed fix(ivy): destroy injector when module is destroyed Jan 16, 2019
kara
kara approved these changes Jan 18, 2019
View reviewed changes
Copy link
Copy Markdown
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 a team January 18, 2019 01:53
@kara kara removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 18, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 18, 2019
@kara kara added the target: major This PR is targeted for the next major release label Jan 18, 2019
@kara
Copy link
Copy Markdown
Contributor

kara commented Jan 18, 2019

presubmit

@kara kara added the action: merge The PR is ready for merge by the caretaker label Jan 18, 2019
@alxhub
Copy link
Copy Markdown
Member

alxhub commented Jan 18, 2019

Considering @kara and @mhevery's approval to be global.

@alxhub alxhub closed this in ab2bf83 Jan 18, 2019
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot 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.

Reviewers

3 more reviewers

@petebacondarwin petebacondarwin petebacondarwin left review comments

@mhevery mhevery mhevery approved these changes

@kara kara kara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes 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.

7 participants

@crisbeto @mary-poppins @kara @alxhub @petebacondarwin @mhevery @googlebot