fix(signals): destroy hook doesn't run in injection context.

[rainerhahnekamp]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/main/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

It fixes a testing issues when a store, which is provided in root, has an onDestroy hook.

[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?

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 commit requires withHook to access the DI outside of onDestroy.

With the main branch, it is very easily to test. Just run the following test:

it('should fail', () => {
  const Store = signalStore(
    { providedIn: 'root' },
    withState({ name: 'Coffee' }),
    withHooks({
      onDestroy() {
        console.log('will throw...');
      },
    })
  );

  TestBed.inject(Store);
});

What is the new behavior?

onDestroy runs outside of the injection context.

I would like to add, that this PR makes withHooks in its usage fully compatible with withMethods, withComputed and are similar extensions that provide access to the DI.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

We add a note the changelog, describe the reasons and that's it. Signal Store is, as far as I know, still in developer preview.

Other information

I left the old notation of withHooks. If that PR would be accepted, I'd suggest to remove that one as well, so that withHooks accepts only a HookSupplier as argument.

[netlify]

✅ Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	e21d748
🔍 Latest deploy log	https://app.netlify.com/sites/ngrx-io/deploys/6590b854f9ee9c0008c6cac1
😎 Deploy Preview	https://deploy-preview-4196--ngrx-io.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[markostanimirovic]

Thanks Rainer! I suggest splitting the bugfix and new feature into separate PRs.

---

Regarding the new signature, it seems useful! But I'd slightly change it in terms of verbosity:

withHooks((store) => {
  // on init logic

  return () => /* on destroy logic */
})

Inspired by: https://svelte.dev/docs/svelte#onmount

Returning the onInit callback seems unnecessary to me because init logic can be performed within the withHooks factory (out of onInit) anyway:

withHooks(() => {
  // init logic can be performed here

  return {
    onInit(store) {
      // and here as well
    }
  };
})

---

Also, feel free to open a feature request for the new withHooks signature, so we can discuss about ideas there.

[rainerhahnekamp]

I'll split it into two parts @markostanimirovic. But please help me out here. What would be the new feature here? As far as I see it, the fix is the new feature.

[markostanimirovic]

I'll split it into two parts @markostanimirovic. But please help me out here. What would be the new feature here? As far as I see it, the fix is the new feature.

We can split it in the following way:

• Bugfix: Executing onDestroy out of injection context with tests that verify the fix
• Feature: Proposal for new withHooks signature

[rainerhahnekamp]

I'm sorry, but I don't know how to provide a fix without breaking change.

Currently, it is like this:

inject(DestroyRef).onDestroy(() => {
  runInInjectionContext(injector, hooks.onDestroy!);
});

The way I understand the proposed fix:

inject(DestroyRef).onDestroy(() => {
  hooks.onDestroy!();
});

But then any hook like

withHooks({onDestroy(store) {
  const service = inject(SomeService);
}});

will fail. So I would fix one thing and introduce a bug at the same time.

Maybe you can give me a hint. Thanks!

[markostanimirovic]

I'm sorry, but I don't know how to provide a fix without breaking change.

Currently, it is like this:

inject(DestroyRef).onDestroy(() => {
  runInInjectionContext(injector, hooks.onDestroy!);
});

The way I understand the proposed fix:

inject(DestroyRef).onDestroy(() => {
  hooks.onDestroy!();
});

But then any hook like

withHooks({onDestroy(store) {
  const service = inject(SomeService);
}});

will fail. So I would fix one thing and introduce a bug at the same time.

Maybe you can give me a hint. Thanks!

Yeah, it will be a breaking change, but if I understood correctly, the current behavior breaks tests when SignalStore is provided at the root level, so I think it's fine to move the onDestroy execution out of the injection context and provide the additional withHooks signature in a separate PR once we agree about its design. Both - feature and bugfix can be released together.

Btw, @ngrx/signals is in developer preview, so we can introduce breaking changes in minor/patch releases as Angular does with signals. Of course, breaking changes will be reduced to a minimum, but this one seems necessary.

[rainerhahnekamp]

This one gets split up into two different PRs (fix and feature)