`@ngrx/signals`: Allow returning an array of observables from `withEventHandlers`

[markostanimirovic]

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

signals

Information

The withEventHandlers factory (renamed from withEffects in v21) expects a dictionary of observables:

withEventHandlers((store, events = inject(Events)) => ({
  logType$: events.on(increment).pipe(
    tap(console.log)
  ),
})),

While this should still be the preferred way for user-defined handlers for better readability, the ability to return an array of observables will provide additional flexibility and reduce overhead in some cases:

withEventHandlers((store, events = inject(Events)) => [
  events.on(increment).pipe(tap(console.log)),
]),

Describe any alternatives/workarounds you're currently using

No response

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

• Yes
• No

[manfredsteyer]

I like this take. Perhaps, we could further align the syntax with the syntax we use for reducers:

withEventHandlers(    
    on(increment).pipe(tap(console.log)),
    on(decrement).pipe(tap(console.log)),
),

This would make the API more symmetric.

[rainerhahnekamp]

Funny, just used it in the morning and thought if we can't come up with an even better syntax 😀

I'd also say that an option where we don't return anything would be even better than an array.

[markostanimirovic]

I like this take. Perhaps, we could further align the syntax with the syntax we use for reducers:

withEventHandlers(    
    on(increment).pipe(tap(console.log)),
    on(decrement).pipe(tap(console.log)),
),

This would make the API more symmetric.

@manfredsteyer The main problem with this signature is accessing the store instance and injecting other services, which is required in most cases for handlers/effects. To enable this, we would still need to use a callback and return an array:

withEventHandlers(
  (
    store,
    dataService = inject(DataService),
  ) => [
    on(increment).pipe(tap(console.log)),
    on(decrement).pipe(tap(console.log)),
  ]
),

After this (required) change, the overhead in the on function implementation to serve both the reducer and handlers won't be worth it, in my opinion, because there is not a big difference in developer experience compared to explicit injection of the Events service:

withEventHandlers(
  (
    store,
    events = inject(Events),
    dataService = inject(DataService),
  ) => [
    events.on(increment).pipe(tap(console.log)),
    events.on(decrement).pipe(tap(console.log)),
  ]
),

On the contrary, the explicit injection provides the ability to write a custom reducer feature if the default one (withReducer) does not suit some advanced use cases:

withEventHandlers(
  (
    store,
    // injecting ReducerEvents instead of Events
    events = inject(ReducerEvents),
  ) => [
    events.on(increment).pipe(
      tap(() => patchState(store, /* ... */))
    ),
    events.on(decrement).pipe(
      tap(() => patchState(store, /* ... */))
    ),
  ]
),

Note: The ReducerEvents service will be part of the public API in v21, when #4997 gets merged.

[markostanimirovic]

I'd also say that an option where we don't return anything would be even better than an array.

@rainerhahnekamp that's already possible by using withHooks => onInit:

withHooks({
  onInit(store, events = inject(Events)) {
    events.on(increment).pipe(
      tap(console.log),
      takeUntilDestroyed(),
    ).subscribe();

    events.on(decrement).pipe(
      tap(console.log),
      takeUntilDestroyed(),
    ).subscribe();
  }
})

We can potentially provide a helper for this, but I don't see a big benefit compared to takeUntilDestroyed + explicit subscribe:

withHooks({
  onInit(store, events = inject(Events)) {
    eventHandler(
      events.on(increment).pipe(
        tap(console.log),
        takeUntilDestroyed(),
      )
    );

    eventHandler(
      events.on(decrement).pipe(
        tap(console.log),
        takeUntilDestroyed(),
      )
    );
  }
})

[rainerhahnekamp]

@markostanimirovic, all good. I wouldn't do that in the hooks.

I was more for the following version (I guess @manfredsteyer meant the same but skipped the callback part):

withEventHandlers(
  (
    store,
    dataService = inject(DataService),
  ) => {
   on(increment).pipe(tap(console.log)),
   on(decrement).pipe(tap(console.log)),
  }
),

I didn't think about the implementation though. I don't know how that would be even possible...the only chance would be to use the Events explicitly or provide a special on in the callback... like this

withEventHandlers(
  (
    store,
    on,
    dataService = inject(DataService),
  ) => {
   on(increment).pipe(tap(console.log)),
   on(decrement).pipe(tap(console.log)),
  }
),

So yeah, let's pretend that conversation never happened 😅

---

Another thing about the ReducerEvents. Don't we have the risk that users run again into #4857? If yes, I think we should provide a lint rule which gives a warning for that. I'd say if somebody wants to use the ReducerEvents outside of withReducer, then they should really do that in withHooks instead withEffects.

[markostanimirovic]

I was more for the following version (I guess @manfredsteyer meant the same but skipped the callback part):

withEventHandlers(
(
store,
dataService = inject(DataService),
) => {
on(increment).pipe(tap(console.log)),
on(decrement).pipe(tap(console.log)),
}
),
I didn't think about the implementation though. I don't know how that would be even possible...the only chance would be to use the Events explicitly or provide a special on in the callback... like this

withEventHandlers(
(
store,
on,
dataService = inject(DataService),
) => {
on(increment).pipe(tap(console.log)),
on(decrement).pipe(tap(console.log)),
}
),
So yeah, let's pretend that conversation never happened 😅

We would have to reimplement the pipe function to subscribe after piped operators. 😅

Another thing about the ReducerEvents. Don't we have the risk that users run again into #4857? If yes, I think we should provide a lint rule which gives a warning for that. I'd say if somebody wants to use the ReducerEvents outside of withReducer, then they should really do that in withHooks instead withEffects.

I don't think users can run into the mentioned bug again, because that was an issue with event dispatch order/timing, not the event handling part.

The idea of exposing ReducerEvents publicly is to provide full flexibility to library authors. It should be rarely used in applications, because withReducer suits at least 90% of the use cases. I don't think we should prevent flexibility because someone can use some APIs in the wrong way. That could happen anyway. 🤷‍♂️