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(signals): run onDestroy outside of injection context #4200

Conversation

rainerhahnekamp
Copy link
Contributor

@rainerhahnekamp rainerhahnekamp commented Jan 3, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

The Signal Store saves the injection context upon instantiation and re-uses it in the onDestroy hook.

If the Store is provided in root, any test with an onDestroy hook fails with the error message: NG0205: Injector has already been destroyed.

This PR runs onDestroy outside of the injection context.

Since this will break any code which depends on the injection context in onDestroy, there will be follow-up (breaking change) feature which will provide the injection context before the hook is executed.

Discussed in: #4196

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Any test with a store providedIn: root fails.

Closes #

What is the new behavior?

onDestroy runs outside of the injection context.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

There will be a feature PR which re-enables the injection context, but in a different way.

Other information

Copy link

netlify bot commented Jan 3, 2024

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 66994ed
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/6597ca192431f3000820c129

@brandonroberts brandonroberts merged commit e21df19 into ngrx:main Jan 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants