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

fix(core): call ngOnDestroy for tree-shakeable providers #28943

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@JoostK
Copy link
Member

commented Feb 23, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #28927

Prior to this change, any provider that was independently resolved using
its InjectableDef would not be considered when destroying the module it
was requested from.

What is the new behavior?

This commit provides a fix for this issue by storing
the resolved provider in the module's list of provider definitions.

Additionally, a test for Ivy's module injector was added to prevent
regressing on this behavior going forward.

Does this PR introduce a breaking change?

  • Yes
  • No

JoostK added some commits Feb 23, 2019

fix(core): call ngOnDestroy for tree-shakeable providers
Prior to this change, any provider that was independently resolved using
its InjectableDef would not be considered when destroying the module it
was requested from. This commit provides a fix for this issue by storing
the resolved provider in the module's list of provider definitions.

Fixes #28927
test(ivy): verify ngOnDestroy is called for tree-shakeable providers
This test verifies that Ivy's module injector does not suffer from
#28927 to prevent regressing on this behavior going forward.

@JoostK JoostK requested a review from angular/fw-core as a code owner Feb 23, 2019

@googlebot googlebot added the cla: yes label Feb 23, 2019

@@ -109,7 +109,7 @@ export function resolveNgModuleDep(
} else if (
(injectableDef = getInjectableDef(depDef.token)) && targetsModule(data, injectableDef)) {
const index = data._providers.length;
data._def.providersByKey[depDef.tokenKey] = {
data._def.providers[index] = data._def.providersByKey[depDef.tokenKey] = {

This comment has been minimized.

Copy link
@JoostK

JoostK Feb 23, 2019

Author Member

I believe this can not make data._def.providers sparse, as the indices should be aligned. I'd like a second opinion on this though :)

@JoostK

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

/cc @alxhub

@JoostK JoostK changed the title Module ondestroy fix(core): call ngOnDestroy for tree-shakeable providers Feb 23, 2019

@ngbot ngbot bot added this to the needsTriage milestone Feb 23, 2019

@benlesh benlesh removed the comp: ivy label Feb 23, 2019

@mhevery

mhevery approved these changes Apr 4, 2019

@mhevery

This comment has been minimized.

@IgorMinar IgorMinar closed this in 30b0442 Apr 5, 2019

IgorMinar added a commit that referenced this pull request Apr 5, 2019

test(ivy): verify ngOnDestroy is called for tree-shakeable providers (#…
…28943)

This test verifies that Ivy's module injector does not suffer from
#28927 to prevent regressing on this behavior going forward.

PR Close #28943

DeveloperFromUkraine added a commit to DeveloperFromUkraine/angular that referenced this pull request Apr 11, 2019

fix(core): call ngOnDestroy for tree-shakeable providers (angular#28943)
Prior to this change, any provider that was independently resolved using
its InjectableDef would not be considered when destroying the module it
was requested from. This commit provides a fix for this issue by storing
the resolved provider in the module's list of provider definitions.

Fixes angular#28927

PR Close angular#28943

DeveloperFromUkraine added a commit to DeveloperFromUkraine/angular that referenced this pull request Apr 11, 2019

test(ivy): verify ngOnDestroy is called for tree-shakeable providers (a…
…ngular#28943)

This test verifies that Ivy's module injector does not suffer from
angular#28927 to prevent regressing on this behavior going forward.

PR Close angular#28943

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

fix(core): call ngOnDestroy for tree-shakeable providers (angular#28943)
Prior to this change, any provider that was independently resolved using
its InjectableDef would not be considered when destroying the module it
was requested from. This commit provides a fix for this issue by storing
the resolved provider in the module's list of provider definitions.

Fixes angular#28927

PR Close angular#28943

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

test(ivy): verify ngOnDestroy is called for tree-shakeable providers (a…
…ngular#28943)

This test verifies that Ivy's module injector does not suffer from
angular#28927 to prevent regressing on this behavior going forward.

PR Close angular#28943
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.