provide injection context to `onDestroy`

[rainerhahnekamp]

Which @ngrx/* package(s) are relevant/related to the feature request?

signals

Information

As discussed in https://github.com/ngrx/platform/pull/4196, we have to run onDestroy outside of the injection context.

At the moment, there are two options on the table:

Version 1: "Svelte style":

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

  const service = inject(SomeService);

  return () => {
    service.doSomethingAtTheEnd();
  }
})

Version 2:

withHooks((store) => {
  // access to DI

  const service = inject(SomeService);

  return {
    onInit() {
      service.doSomethingAtTheStart();
    },

    onDestroy() {
      service.doSomethingAtTheStart();
    }
  }
})

I opt for version 2 because

1. it follows the same pattern of withMethods & withComputed, where we have the first part which provides access to the DI and the second contains the implementation.
2. It allows us to add further hooks. For example there is a discussion, asking for a onChange: https://github.com/ngrx/platform/discussions/4192
3. Having an explicit onInit might be more familiar to an Angular developer because of the same hook in the framework.

---

In the end, it is a question of personal style. From a technical perspective, both options are ok.

Describe any alternatives/workarounds you're currently using

No response

I would be willing to submit a PR to fix this issue

• [X] Yes
• [ ] No

[ducin]

Hi 👋 On one hand, version 1 resembles svelte and, first and foremost, react's useEffect's cleanup callback, as well as redux subscribe's unsubscribe callback.

On the other hand, such style doesn't seem to be common within angular ecosystem. For instance, signal effect could return a cleanup callback, but it doesn't. Similar to rxjs it returns an object with an explicit cleanup method (effect: destroy, rx: unsubscribe).

My vote is for version 2 which is explicit and slightly more consistent with other cleanup APIs.

[brianpooe]

I vote for version 2

[markostanimirovic]

The thing that I don't like about option 2 is that it leaves some space for 'abuse'/wrong usage:

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

  return {
    onInit() {
      // but also here
    },
  };
});

On the other hand, it is consistent with other base SignalStore features and provides the possibility of potential extension in the future. 👌

withHooks((store, service = inject(MyService)) => ({
  onInit() {},
  onDestroy() {},
}));

[ducin]

@markostanimirovic since the API is closure-based anyway, it's kind of natural that you can access the enclosing scope. Perhaps in corner cases someone would need to share a value (ID? token? whatever) in both onInit and onDestroy. In both versions there is closure access anyway.

[rainerhahnekamp]

@markostanimirovic what is the problem if somebody does

withHooks((store) => {
  const service = inject(Service);

  return {
    onInit() {
      service.doInitStuff();      
    },
  };
});

vs

withHooks((store) => {
  const service = inject(Service);
  service.doInitStuff();      

  return {};
});

It is not consistent but is this such a big thing that we want to enforce it by design? Or are there are thing which might happen, that I am not aware of?

[rainerhahnekamp]

I just realized that version 2 would include a breaking change as well:

withHooks((store) => {
  const service = inject(Service);

  return {
    onInit() {
      service.doInitStuff();      
    },
  };
});

Currently, the hooks get the store as parameter. In the version above, the store is passed on to withHooks.

I am using the non-breaking change version in https://github.com/ngrx/platform/pull/4208 which looks like this:

withHooks(() => {
  const service = inject(Service);

  return {
    onInit(store) {
      service.doInitStuff();      
    },
    onDestroy(store) {
      service.doDestroyStuff();      
    },
  };
});

What should we do? If we want to be consequent, the store should be passed to withHook and not to onInit or onDestroy.

[brandonroberts]

I think the store should be passed to withHooks and not to onInit or onDestroy along with keeping the onInit and onDestroy hooks as separate functions. If we introduce some other hook, it would already have access to the store context also. Its also consistent with how these lifecycle hooks have worked in Angular like ngOnInit without provided arguments.

We can't prevent everyone from abusing the usage, but we can potentially add some lint rules around certain usage to sway people away from it.

[rainerhahnekamp]

The PR has been updated with store being passed to withHooks.