yet another attempt at actionListenerMiddleware

[phryneas]

Reference: #237, #432, #272 .

So this is just the middleware and a few tests for it, I have not yet thought about how to usefully combine that into createDefaultMiddleware or even configureStore.

Let's discuss the middleware itself for now and take a look at that later.

The tests should showcase all functionality:

• subscribing via method on the middleware
• unsubscribing via method on the middleware
• unsubscribing via returned callback from subscribing
• subscribing via action
• unsubscribing via action
• unsubscribing via callback returned from dispatch

Also, I've added the following options to each individual subscription (taken some extra inspiration from https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener):

interface ActionListenerOptions<
  A extends AnyAction,
  S,
  _ extends Dispatch<AnyAction>
> {
  /**
   * Indicates that the listener should be removed after if has run once.
   */
  once?: boolean
  /**
   * If set to true, the action will not be forwarded to
   * * listeners that were registered after this listener
   * * middlewares later in the middleware chain
   * * reducers
   * If this listener is skipped due to `options.condition`, this has no effect.
   */
  preventPropagation?: boolean
  /**
   * A function that determines if the listener should run, depending on the action and probably the state.
   */
  condition?(action: A, getState: () => S): boolean
}

So, let's discuss this :)

[netlify]

Deploy preview for redux-starter-kit-docs ready!

Built with commit cddf92e

https://deploy-preview-547--redux-starter-kit-docs.netlify.app

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cddf92e:

Sandbox	Source
hardcore-sky-hyn68	Configuration
practical-hill-l1wyb	Configuration
youthful-frost-76g0z	Configuration

[markerikson]

Would we actually want to forward the addListener action onwards at all?

[phryneas]

I do have something in the back of my mind that someone talked about maybe wanting to track all active subscriptions in a state or so - in that case it would definitely be useful. On the other hand, the delete action.meta would be counter-productive then.

Also, if it is passed thorugh, it can be observed in the devTools, wich might be nice.

[mpeyper]

Yeah, that was me (back when I still thought I'd have the time to look at this 😓), although I did concede that in practise, nothing ever actually reacted to those events. The observability in the dev-tools has saved us on multiple occasions, so I'd advocate for keeping it.

The only thing I'd recommend is cleansing the action of any functions so that it serialisable by the time it hit the store. I thought RTK actually raised a warning if the action wasn't?

[mpeyper]

Just noticed the the delete action.meta.

Not sure if you want to modify the original action in case the dispatcher is reusing the same action (i.e. if they're adding and removing listeners on component mount/dismount).

[markerikson]

Is there really a meaningful benefit to having actions like "add/remove listener" show up in the DevTools? I've never been a fan of the idea of dispatching actions just to get a logging effect.

[phryneas]

Honestly, 90% of the time I look into the devtools to just look at the action names (sometimes payloads) to get a feeling for the flow what happened.
10% for state changes. Never use time travel.
So I'm very much in favor of forwarding it, but I agree that it has to have some use.

Right now I'm thinking about preventing the "listen" action from propagating and dispatching a new "started_listening" action, which might also contain a unique id that could be used to unsubscribe that listener.

[phryneas]

Hmm. While trying to implement this "started_listening", it seems non-trivial:

• do we dispatch that action only when it is subscribed via addListener action?
• if we also dispatch it, what if the middleware isn't used by a store yet - or used by multiple stores?
• would we dispatch something different if a listener was not added because it was already in use? Same for removal.

So for now, I'm tending towards cutting that feature altogether. Let's get this out as simple as possible, we can always add it back later on, with some separate discussions. Right now it looks like it might not be worth the amount of work (and complexity) that would be required.

[phryneas]

Removed it for now. Let's re-evaluate that later, in a separate issue.

[markerikson]

Just updated the PR by moving the files from src/entities to src, and exporting the middleware. That way we at least get a usable build out of CodeSandbox.

[nogwater]

I started playing with this in codesandbox, starting from priceless-gates-bu5x2. I made a simple counter component that listens for the displayRepo, increments the count, and displays the current count. I updated store.ts with:

const actionListenerMiddleware = createActionListenerMiddleware()
export const addActionListener = actionListenerMiddleware.addListener

const middleware = [...getDefaultMiddleware(), actionListenerMiddleware]

const store = configureStore({
  middleware,
  reducer: rootReducer
})

Is this a good way to expose actionListenerMiddleware.addListener?

For the counter itself, I initially added this:

  addActionListener(displayRepo, action => {
    console.log(action)
    setCount(count + 1)
  })

The count works as I'd want, but I noticed that it prints an additional log entry every time I click Load Repo. So, the first time I click it, it prints one entry, the second time it prints two entries, and so on. Clicking Load Repo is causing my Counter component to rerender, which causes a new listener to be registered.

To fix it, I used useEffect to unsubscribe the previous listener each time I add a new one:

  useEffect(() => {
    const unsub = addActionListener(displayRepo, action => {
      console.log(action)
      setCount(count + 1)
    })
    return unsub
  }, [count, setCount])

Is this what I should be doing? Should I make a custom useActionListener hook that handles the useEffect boilerplate?

[markerikson]

@nogwater : the middleware has the ability to add listeners by dispatching an action, so you don't have to import that method from the middleware itself:

const unsubscribe = store.dispatch(addListenerAction(testAction1, listener))

For this specific example, I'd suggest changing the setter call to use the functional updater form:

setCount(count => count + 1)

That way you don't have to depend on count in the deps array, and you can just add the listener once on mount (ie, an empty deps array). Also, you don't have to have setCount in the deps array, because useState setters are always a stable reference.

It's interesting you jumped straight to doing that in a component. @phryneas had specific concerns about that as a possible anti-ptatern at #237 (comment) . My thought a couple comments later was that I'm not overly concerned about that myself.

[nogwater]

@markerikson Thanks for the tips, and pointing to comments. I'll probably take a crack at rebuilding this without putting the subscription in the component tomorrow. Until then, I tried applying your suggestions and ended up with a couple warnings.

  const dispatch = useDispatch()
  useEffect(() => {
    const unsubscribe = dispatch(
      addListenerAction(displayRepo, () => setCount(count => count + 1))
    )
    return unsubscribe
  }, [])

The first TS is that unsubscribe's type, AddListenerAction<...>, isn't compatible with what useEffect expects for a callback, EffectCallback. The second is that eslint wants dispatch added to the dependencies.

[markerikson]

Yep, [dispatch] is a normal deps requirement, because the lint rule doesn't know that is going to stay stable. (Technically it could switch if you swapped out the store instance, but the lint rule doesn't actually know that either.)

Looking at the React types, I see:

    // NOTE: callbacks are _only_ allowed to return either void, or a destructor.
    // The destructor is itself only allowed to return void.
    type EffectCallback = () => (void | (() => void | undefined));

So technically EffectCallback is the callback function you're passing to useEffect itself, not the cleanup function you return.

I think part of the issue here is that dispatch(), by default, returns the action object you've passed in. Middleware can change that, but you have to modify the store type to make TS know that...

Oh. Oh no.

@phryneas !!!!! Guess what fun type issue we just ran into?

THE EXACT SAME THING I WAS DOING LAST YEAR WITH CONVINCING TS THAT DISPATCHING A SPECIFIC ACTION MAKES dispatch ACTUALLY RETURN A PROMISE! :)

It thinks that unsubscribe is the action we dispatched, not the cleanup function the middleware returns, and from TS's perspective an "action object" is certainly not the same type as a "function that returns void".

I know you'd come up with a nicer way to handle that, and I think I'd used that technique in my work project, but I don't have it handy right this second. Do you remember what it was? If not, I'll have to look it up later.

[phryneas]

Heh, you ran into my type-related TODO. Dispatch return type isn't correct automatically.

redux-toolkit/src/entities/createActionListenerMiddleware.test.ts

Line 119 in cf44723

// @ts-ignore TODO types

For now, just add a // @ts-ignore above the return, I'm gonna fix it before it's ready.

[phryneas]

The count works as I'd want, but I noticed that it prints an additional log entry every time I click Load Repo. So, the first time I click it, it prints one entry, the second time it prints two entries, and so on. Clicking Load Repo is causing my Counter component to rerender, which causes a new listener to be registered.

To fix it, I used useEffect to unsubscribe the previous listener each time I add a new one:

  useEffect(() => {
    const unsub = addActionListener(displayRepo, action => {
      console.log(action)
      setCount(count + 1)
    })
    return unsub
  }, [count, setCount])

Yup, you'd have to use a useEffect with an unsubscribe callback, because while the middleware prevents the same listener from being added twice, if you define the listener function within the render method, it will be a different function each time - so both functions would be subscribed since they're distinct from each other.

[nogwater]

The next(action) call occurs after calling the actions with entry.listener(action, api). This means that you can't have a listener that uses the updated state, right? Is this intentional? I found #237 (comment) that says the reducers should run first. The logic for this block might get tricky with checking conditions and preventPropagation, but might still be doable.

[phryneas]

Phew. That's a good point.
Ideally, we'd like to allow for both behaviours.

I could imagine adding an option { when: 'before' | 'after' }, defaulting it to before and issuing an Error from the stopPropagation method when called in combination with after.
What do you think?

[phryneas]

Ouch. This one hurts quite a bit in the typings. Following through with it nevertheless ^^

[phryneas]

Okay, that's in. Comments? :)

[nogwater]

If when is after, should the entry.condition be given the previous state or the next state?
You are returning from within the for loop which means not all listeners will be processed.

This is a bit verbose, and totally untested, but how about something like:

if (listeners) {
  // process 'before' listeners
  for (const entry of listeners) {
    if (entry.when == 'after') {
      continue;
    }

    if (!entry.condition || entry.condition(action, api.getState)) {
      if (entry.once) {
        listeners.delete(entry)
      }

      let stoppedPropagation = false
      entry.listener(action, {
        ...api,
        stopPropagation() {
          stoppedPropagation = true
        }
      })
      if (stoppedPropagation) {
        return action
      }
    }
  }

  const result = next(action)

  // process 'after' listeners
  for (const entry of listeners) {
    if (entry.when != 'after') {
      continue;
    }

    if (!entry.condition || entry.condition(action, api.getState)) {
      if (entry.once) {
        listeners.delete(entry)
      }

      entry.listener(action, {
        ...api,
        stopPropagation: () => {
          throw new Error(
          'stopPropagation can only be called by action listeners with the `when` option set to "before"'
          )
        }
      })
    }
  }
  return result
}
return next(action)

I think this leaves the door open for having a when: both option, but I don't know why someone would want that.

[phryneas]

Personally, I'd like to have the ability to stopPropagation by default. Feels more en par with DOM events, onClick listeners and the likes.

I believe in sagas it's only the case because those are async AF and usually a million ticks have happened yielding 80 times before anything meaningful happens. (Doesn't sound like it, but I still really like saga ;) )

@markerikson do you have an opinion on this?

[markerikson]

Both sagas and observables explicitly let the action hit the reducers first:

https://github.com/redux-saga/redux-saga/blob/a8c968b77243f59fbe672ca56691761664e060bf/packages/core/src/internal/middleware.js#L27

https://github.com/redux-observable/redux-observable/blob/dfef883fdba9f669ea88139bdb7abd2d026ff7a8/src/createEpicMiddleware.ts#L88-L91

I'd say that's the preferable default.

[mpeyper]

Just a thought, could the listener behave a bit like useEffect and allow you to return a function to run after the state update? something like:

function myListener(action, { getState }) {
  let previousValue = getState().valueWeCareAbout
  return () => {
    let  newValue = getState().valueWeCareAbout
    if (previousValue === 'Pending' && newValue === 'Failed') {
      logger.error('Failed to submit')
    }
  }
}

Then you could have before listener

const beforeListener = () => {}

and after listener

const afterListener = () => () => {}

or a before and after listener

const bothListener = () => {
  return () => {}
}

[markerikson]

That also feels kinda complex, and the similarity in syntax with useEffect would actually seem like a reason to not do it that way because they're two very different things.

[mpeyper]

I guess I don't see them as very different in terms of wanting to perform logic around state changing (dependency array for useEffect, redux state for this). I could also argue that this primary use case for this middleware is to use effects in response to actions.

There are also some similarities in the api with exisiting redux concepts with thunks and middleware. It could be considered a middleware-lite api so converting your listener to a custom middleware when the requirements grow is eased a bit.

[phryneas]

Oh, btw. I wanna have the features on this finalized before I add type tests - those might add quite some type changes and I don't really want to do that multiple times.

So, the types might change a lot later on.

[phryneas]

@phryneas !!!!! Guess what fun type issue we just ran into?

Fixed it. Weirdest thing. TS can infer everything just fine from a

Middleware<
  {(action: "foo") => "bar"},
  RootState
>

but once you add additional properties (in our case addListener and removeListener) on top, it falls back to Middleware<unknown, unknown>, so it loses the dispatch signature.

Let's just say I found a way to shove it somehow into TypeScript by force.

[markerikson]

The logic for the "before" and "after" cases is almost identical. Is it worth trying to deduplicate it?

[nogwater]

The duplicated code is pretty readable, but here's a stab at deduping it.

  const defaultStage = 'before'
  if (listeners) {
    const triggerListener = (stage, listener) => {
      let stoppedPropagation = false

      if (
        (listener.when || defaultStage) === stage &&
        (!listener.condition || listener.condition(action, api.getState))
      ) {
        if (listener.once) {
          listeners.delete(listener)
        }

        listener.listener(action, {
          ...api,
          stopPropagation() {
            if (stage === 'after') {
              throw new Error(
                'stopPropagation can only be called by action listeners with the `when` option set to "before"'
              )
            }
            stoppedPropagation = true
          },
        })
      }

      return stoppedPropagation
    }

    /* before */
    for (const entry of listeners) {
      const stoppedPropagation = triggerListener('before', entry)
      if (stoppedPropagation) {
        return action
      }
    }

    const result = next(action)

    /* after */
    for (const entry of listeners) {
      triggerListener('after', entry)
    }

    return result
  }
  return next(action)

[phryneas]

those are now deduplicated & shrunk quite a bit

[mpeyper]

Is there a particular feature this is satisfying that just having the condition around the body of the listener function wouldn't also provide?

function myCondition(action, getState) {
  // ...
}

function myListener(action, { getState } ) {
  // ...
}

store.dispatch(addListener(myListener, { condition: myCondition }))

vs.

function myCondition(action, getState) {
  // ...
}

function myListener(action, { getState } ) {
  if (myCondition(action, getState)) {
    // ...
  }
}

store.dispatch(addListener(myListener))

Similarly to reducers, a higher-order listener could be used it the same condition logic was needed or if the listener was being provided by a third party and I wanted to add a condition to it

import someListener from 'some-dep'

function myListener(action, ...args) {
  if (action.type !== 'secret action type') {
    return someListener(action, ...args)
  }
}

store.dispatch(addListener(myListener))

I just feels like it's adding complexity to the internals without provide anything they can do (and relatively easily) in userland... unless I'm missing something?

[markerikson]

Hmm. Yeah, I could see it being straightforward to do conditional and once handling on your own, although we might need to document that as recipes.

[phryneas]

Hm. I'd really like to keep the once option, as having a "react to only the next action of type X" seems really useful sometimes.
In that context, the condition also has it's value. So I'm not really sure if I would want to let got of that.

[phryneas]

An alternative option to once could be to add an unsubscribe function to the api. Not 100% if I like the idea yet. Thoughts?

[mpeyper]

Could a conditional once listener not be implemented as

function myListener(action, { getState, dispatch } ) {
  if (myCondition(action, getState)) {
    dispatch(removeListener(myListener))
    // ...
  }
}

[phryneas]

Yes, but that's a lot less obvious to the user than

function myListener(action, { getState, unsubscribe } ) {
  if (myCondition(action, getState)) {
    unsubscribe()
  }
}

so I'd rather go that way, as it doesn't really cost much to add that.

[markerikson]

Lotta good discussion here. I haven't assimilated all of it, tbh, but a few quick thoughts:

• "After" is definitely the typical approach. I don't immediately see a lot of reason to run stuff "before".
• Similarly, while I can see some point to the "stop propagation" stuff, it feels like there's a bunch of edge cases there that we ought to avoid for now
• On the flip sides, this means that even "signal actions" meant to trigger more logic really still have to be serializable or else the dev check middleware will yell at you. They also trigger a reducer run and subscriber notifications.
• I am curious if there's maybe value in providing access to the previous state besides the updated state

So, my inclination would be that we scale this back as an MVP. Drop "before", run everything after, drop stopPropagation. Keep it simpler for now, and if there's really an expressed desire for more flexibility later, we can add the options back in as people give us more concrete use cases.

Thoughts on adding getPreviousState to the options?

[nogwater]

Thoughts on adding getPreviousState to the options?

Do getPreviousState and getState need to be a passed as functions for listeners? It looks like the reason for getState is a function is to check that it's not being accessed from a reducer, but if we are passing the state to a listener we already know we aren't in a reducer. Could the api be { dispatch, currentState, previousState } ? (I don't know if i like currentState, state, or nextState.) I assume to make getPreviousState work, we'd need to call getState before next(action), save the results, and then wrap it in a function.

[markerikson]

Hrm. Y'know, that brings up an interesting point.

By passing in getState to the listener, we enable them to read the "current" state at any point in the future, ie, in the middle of some async/await stuff. So yeah, we definitely want getState.

What doesn't give you is the ability to know what the state was before the listener even got triggered at all, which is why I was thinking about getPreviousState.

As a side note: these listeners are definitely way weaker than sagas and observables, in that I don't see how you'd listen for multiple distinct actions in sequence. I'm okay with that, as this is deliberately intended to be a much more scoped API anyway.

[mpeyper]

As a side note: these listeners are definitely way weaker than sagas and observables, in that I don't see how you'd listen for multiple distinct actions in sequence. I'm okay with that, as this is deliberately intended to be a much more scoped API anyway.

Yeah, I definitely agree that that we don't want to offer a complete competitor to these solutions, but rather a lightweight alternative or stepping stone for the basic use cases.

I have previously stated my preference to allow listeners to react to multiple action types as a way of tracking more complex processes being undertaken, but I'm happy with concede the point as it's technically possible for a listener to subscribe new listeners or that that is a job for the more powerful alternatives.

On that point, the current proposal has included an unsubscribe function in the listener API to simplify "one shot" listeners, so I'm wondering if a similar subscribe function to start listening again should also be included. This would allow something like this to be possible in a fairly easy to read an understand way:

function createCompletionListener(onCompletion) {
  return (action, { unsubscribe }) => {
    unsubscribe()
    onCompletion()
  }
}

function triggerListener(action, { getState, dispatch, subscribe, unsubscribe }) {
  unsubscribe()

  const completionListener = createCompletionListener(() => {
    subscribe()
    const { processState } = getState()
    dispatch({ type: "PROCESS_COMPLETE", payload: processState })
  })
  dispatch(addListener('FINAL_PROCESS_ACTION', completionListener))
}

store.dispatch(addListener('FIRST_PROCESS_ACTION', triggerListener))

What doesn't give you is the ability to know what the state was before the listener even got triggered at all, which is why I was thinking about getPreviousState.

Getting access to the previous state would be useful for a few things. Responding to a change in state over time, enabling some kind of snapshot/restore functionality or generating a diff to report distinct changes in a log message come to mind.

My only concern is when there is async work happening, the getPreviousState will always return the previous state from when it triggered, not from before the last dispatch or any other weird way users might interpret the word "previous" in the context they are calling it. It might just be a naming thing (I don't have an alternative that isn't overly complex and verbose - getStateBeforeTrigger) or perhaps I'm just not giving people enough credit?

[markerikson]

One other potential wrench in the gears:

Do we want to add extraArgument support ala thunks? Is that something that would make sense?

[mpeyper]

When I was looking at an implementation of this, I had included a withExtraArgument option to the API as I was trying to have a very thunk-esque API. I personally like the idea of keeping it familiar to thunk users, especially given that thunks are also supported by default in RTK.

Selfishly, we currently use the withExtraArgument feature of thunks where I work to inject preconfigured axios instances (for dealing with the previously mentioned 2FA feature, as well as some other global functionality) and when to do some finessing of the sagas (barely documented) context feature, so having the option here as well would make my life easier.

With my own experimentation, the extra argument was added when creating the middleware:

  let store = configureStore({
    reducer: () => ({}),
    middleware: [createActionListenerMiddleware(initialListeners).withExtraArgument({ api })]
  })

In hindsight, I'd probably not worry with changing functions (trying too hard to be exactly like thunk) and just let it be passed into the createActionListenerMiddleware function directly, or as an options argument if we want to leave it open for more options in the future:

  let store = configureStore({
    reducer: () => ({}),
    middleware: [createActionListenerMiddleware(initialListeners, { api })]
  })

  let store = configureStore({
    reducer: () => ({}),
    middleware: [createActionListenerMiddleware(initialListeners, { extraArgument: { api } })]
  })

Although, I've just double checked the implementation here and it seems as though you cannot actually create the middleware with any listeners, so the thunk-like API could still be beneficial if the middleware was exported without needing to be created:

  let store = configureStore({
    reducer: () => ({}),
    middleware: [
      thunk.withExtraArgument({ api }),
      listeners.withExtraArgument({ api })]
  })

I'd also be interested in exploring an option to include the extra argument (or additional extra arguments?) when adding a listener, This would enable my above example of a short term listener to be reduced to:

function completionListener(action, { unsubscribe }, onCompletion) {
    unsubscribe()
    onCompletion()
  }
}

function triggerListener(action, { getState, dispatch, subscribe, unsubscribe }) {
  unsubscribe()

  const onCompletion = () => {
    subscribe()
    const { processState } = getState()
    dispatch({ type: "PROCESS_COMPLETE", payload: processState })
  }

  dispatch(addListener('FINAL_PROCESS_ACTION', completionListener, {
    extraArgument: onCompletion
  ))
}

store.dispatch(addListener('FIRST_PROCESS_ACTION', triggerListener))

This may also alleviate some of the gymnastics required to avoid the plain action problem stopPropogation was intended to avoid:

function listener(action, { dispatch }, effectMap) {
    effectMap[action.payload]().then(dispatch)
  }
}

const effectMap = {
  someEffect() {
    return getPromise()
  }
  someOtherEffect() {
    return getAnotherPromise()
  }
}

store.dispatch(addListener(action, listener, { extraArgument: effectMap })

store.dispatch(action('someEffect'))
store.dispatch(action('someOtherEffect'))

[phryneas]

Hmm. I've been thinking about this for a while.

We all seem to have very different things in mind when talking about "action listeners".

What about avoiding that term right now. Not sure if I like the name, given that it is loaded with meaning by react, but for now, what about "action effects" or even "action side effects"?

We could go with a minimal implementation, which means that "effects" are always triggered after an action has been handled by the store and reflect the state after. We could give access to the "before" state, but I guess if people were to implement something that benefitted from that, they were doing something more complex that might warrant a hand-written middleware, so I'd personally leave that out for now.

We could always later - when we see the use case coming up in the issues multiple times with valid use cases - add the concept of "action interceptors" or something like that for what "before" listeners where in my initial suggestion.

Splitting the naming up that way would make it very clear what these things do and avoid the confusion we were having here.

Also, I might have found a way of doing wildcard-like action matches. We could allow to pass a type guard as the first argument to what is now addListener. (as an alternative, not a replacement, for the action creator and the string type that are currently allowed)

  <A extends AnyAction, S, D extends Dispatch, O extends ActionListenerOptions>(
    typeGuard: (a: any) => a is A,
    listener: ActionListener<A, S, D, O>,
    options?: O
  ): AddListenerAction<A, S, D, O>

So that might be used in a simple way to just match multiple possible actions like this (we could provide something like oneOf, with overloads for more than two method arguments - like compose)

const actionA = createAction<string>('a')
const actionB = createAction<number>('b')

function oneOf<T1, T2>(f1: (a: any) => a is T1, f2: (a: any) => a is T2) {
  return (a: unknown): a is T1 | T2 => f1(a) || f2(a)
}

// ends up with the signature `(a: any) => a is (ReturnType<typeof actionA> | ReturnType<typeof actionB>)`
const isAorB = oneOf(actionA.match, actionB.match)

addListenerAction(isAorB, action => {
  // action is inferred to `ReturnType<typeof actionA> | ReturnType<typeof actionB>` here
})

of for more "wildcardy" matches that would still be strongly typed

function hasError(a: any): a is (AnyAction & { meta: { error: any }}) {
  return !!(a?.meta?.error)
}

addListenerAction(hasError, action => {
  // action is inferred to have `meta.error: any`
})

Even regex matches and stuff would be possible that way - leaving the nature of the actual checks to the user's discretion.

PS: extra or context would definitely be a good idea.

[examosa]

Just thought I'd chime in with another library that might provide more inspiration for the api.
Slightly different approach where rather than listening for any specific action, the middleware waits for the provided condition to evaluate to true, providing access to the dispatching action and next state. Then, rather than the reacting function receiving the dispatch, the middleware just dispatches whatever is returned (action, thunk, promise, etc.).
A naming alternative to "action listener" that just popped into my head: "whence" which is both a cheeky combo of when + once and refers to the reacting function (createAction in this library) returning something to the dispatch whence the original triggering action came.

[mpeyper]

I kinda like that redux-when API. It's basic and simple and doesn't have much crossover with the redux-thunk use cases (e.g. it's not trying to handle API calls). I can see this being used to orchestrate actions and thunks into complex sequences quite nicely.

The one thing I'm not sure it could handle is cancelling a promise, which is one of the main drawbacks of thunks vs something like saga or observable.

A naming alternative to "action listener" that just popped into my head: "whence"

I see what where your going with that, but it doesn't work for me. I don't get to make the decisions here though, so you might still have a chance.

[jakobo]

After chatting with @markerikson in #redux, he pointed me over to this ticket. I was able to get the middleware working, but ran into a few API rough spots. (examples in untyped JS)

Listeners as a Set, not Map
Unless we're exposing a builder similar to what's done in createReducer / createSlice, the listener collection will need to be a Set(). I ran into this rough edge because using rtkquery, we only receive a series of action matchers. Since these don't collapse neatly into strings for the listenerMap I changed things over to a Set. There's a small performance hit from needing to filter the listener collection, but it seemed negligible in a prototype with about a hundred side effect listeners.

// expected to work. for example, calling more data in response to an API call
const actionListener = createActionListenerMiddleware();
actionListener.addListener(api.endpoints.getSomething.matchFulfilled, (action, api) => { /*...*/});

Listener creator syntax
The createSlice is where I'd ultimately expect listeners to go, similar to how we auto-generate the reducers, actions, etc. But since we're trying not to change createSlice with this, I just manually attached my listeners to my slice after generation. This resulted in needing some way to quickly define listeners. I extracted prepare since it's used for our standalone action creators, and for a custom createListener function with the signature createListener(typeOrActionCreatorOrMatcher, listener) which uses the same prepare internally for consistency.

function prepare(typeOrActionCreator, listener, options) {
  const type =
    typeof typeOrActionCreator === "string" ||
    typeof typeOrActionCreator === "function"
      ? typeOrActionCreator
      : typeOrActionCreator.type;

  return {
    payload: {
      type,
      listener,
      options,
    },
  };
}

export const createListener = (typeOrActionCreatorOrMatcher, callback) =>
  prepare(typeOrActionCreatorOrMatcher, callback).payload;

export const addListenerAction = createAction(
  "actionListenerMiddleware/add",
  prepare
);

export const removeListenerAction = createAction(
  "actionListenerMiddleware/remove",
  prepare
);

Bulk addListeners on initialization of middleware
Low priority, but with a lot of listeners, the process of adding listeners results in a lot of addListener calls. I added a default listeners to an option bag as part of createActionListenerMiddleware

function createActionListenerMiddleware({listeners: initialListeners = []} = {}) {

---

Generally, the middleware is easy to work with. There weren't too many runtime scenarios where I needed to add and remove listeners from the operation. Most commonly I needed to take an API result from RTKQ and do some follow-up async processing in a manner that would have made queryFn a very scary bit of code. This provided an obvious place for that lightweight side effect logic.

[FaberVitale]

Sorry, I'm a bit late :)

Issue

Have you considered what happens if a listener throws an exception?

In the current implementation an exception raised in a listener would act like a stopImmediatePropagation.

I do not think that is desirable to to have this behaviour especially when an exception is thrown unintentionally (e.g. cannot read property X of undefined).

In big apps it is extremely confusing and frustrating when an unrelated piece of code causes bugs to a feature.

Example

listeners = [
  trackUserClick, // analytics e.g. call gtag or something similar...
  respondToUserClick // business logic
];

1. User interaction triggers an event.
2. trackUserClick raises an exception for some reason:
   integration error server down, cannot read property X of undefined, bug etc...
3. respondToUserClick does not run, we have failed to respond to user interaction due to implicit stopImmediatePropagation behaviour.

---

Other event listener APIs

DOM addEventlistener

Each listener runs in its own task, a subscriber exception does not stop the propagation.

Svelte stores

It does not handle subscriber exceptions, unfortunately.

Observer proposal

A subscriber cannot stop the propagation

rxjs

A subscriber cannot stop the propagation errors are reported in another task

Suggested changes

I believe this event loop should call every listener in a try catch and should then notify the errors.

A global report handler should suffice, consider redux-saga onError as an example,
but every listener could also have the option to provide an error handler.

The latter is implemented in this event listener middleware I wrote recently.

[markerikson]

FYI, I just had a reason to want to try out this middleware in my own app at work. Since it's not actually published yet, and this branch is stale relative to master, I decided to try copy-pasting the logic into my app directly.

I ended up making a few changes as well:

• Since this imports directly from several RTK-internal types, I had to copy-paste those
• As written here, it only supports listening for specific action type strings, either by passing the type string directly, or by passing an RTK-generated action creator with a .type field. I immediately had a need to use RTK's "matchers" instead, so I went ahead and updated the middleware to support that. (This also required that I google up how to infer the generic type result from a type guard, in order to infer the correct action type from a matcher :) )

I'll paste the entire implementation in here for visibility. Maybe we can go ahead and publish this standalone version as the separate package for now?

import { createAction, PayloadAction, Middleware, Dispatch, AnyAction, MiddlewareAPI, Action } from '@reduxjs/toolkit';


interface BaseActionCreator<P, T extends string, M = never, E = never> {
  type: T;
  match(action: Action<unknown>): action is PayloadAction<P, T, M, E>;
}

interface TypedActionCreator<Type extends string> {
  (...args: any[]): Action<Type>;
  type: Type;
}

type MatchFunction<T> = (v: any) => v is T;

export interface HasMatchFunction<T> {
  match: MatchFunction<T>;
}

export const hasMatchFunction = <T>(v: Matcher<T>): v is HasMatchFunction<T> => {
  return v && typeof (v as HasMatchFunction<T>).match === 'function';
};

/** @public */
export type Matcher<T> = HasMatchFunction<T> | MatchFunction<T>;

const declaredMiddlewareType: unique symbol = undefined as any;
export type WithMiddlewareType<T extends Middleware<any, any, any>> = {
  [declaredMiddlewareType]: T;
};

export type When = 'before' | 'after' | undefined;
type WhenFromOptions<O extends ActionListenerOptions> = O extends ActionListenerOptions ? O['when'] : never;

/**
 * @alpha
 */
export interface ActionListenerMiddlewareAPI<S, D extends Dispatch<AnyAction>, O extends ActionListenerOptions>
  extends MiddlewareAPI<D, S> {
  stopPropagation: WhenFromOptions<O> extends 'before' ? () => void : undefined;
  unsubscribe(): void;
}

/**
 * @alpha
 */
export type ActionListener<A extends AnyAction, S, D extends Dispatch<AnyAction>, O extends ActionListenerOptions> = (
  action: A,
  api: ActionListenerMiddlewareAPI<S, D, O>
) => void;

export interface ActionListenerOptions {
  /**
   * Determines if the listener runs 'before' or 'after' the reducers have been called.
   * If set to 'before', calling `api.stopPropagation()` from the listener becomes possible.
   * Defaults to 'before'.
   */
  when?: When;
}

export interface AddListenerAction<
  A extends AnyAction,
  S,
  D extends Dispatch<AnyAction>,
  O extends ActionListenerOptions
> {
  type: 'actionListenerMiddleware/add';
  payload: {
    type: string;
    listener: ActionListener<A, S, D, O>;
    options?: O;
  };
}

/**
 * @alpha
 */
export const addListenerAction = createAction(
  'actionListenerMiddleware/add',
  function prepare(
    typeOrActionCreator: string | TypedActionCreator<string>,
    listener: ActionListener<any, any, any, any>,
    options?: ActionListenerOptions
  ) {
    const type =
      typeof typeOrActionCreator === 'string'
        ? typeOrActionCreator
        : (typeOrActionCreator as TypedActionCreator<string>).type;

    return {
      payload: {
        type,
        listener,
        options
      }
    };
  }
) as BaseActionCreator<
  {
    type: string;
    listener: ActionListener<any, any, any, any>;
    options: ActionListenerOptions;
  },
  'actionListenerMiddleware/add'
> & {
  <C extends TypedActionCreator<any>, S, D extends Dispatch, O extends ActionListenerOptions>(
    actionCreator: C,
    listener: ActionListener<ReturnType<C>, S, D, O>,
    options?: O
  ): AddListenerAction<ReturnType<C>, S, D, O>;

  <S, D extends Dispatch, O extends ActionListenerOptions>(
    type: string,
    listener: ActionListener<AnyAction, S, D, O>,
    options?: O
  ): AddListenerAction<AnyAction, S, D, O>;
};

interface RemoveListenerAction<A extends AnyAction, S, D extends Dispatch<AnyAction>> {
  type: 'actionListenerMiddleware/remove';
  payload: {
    type: string;
    listener: ActionListener<A, S, D, any>;
  };
}

/**
 * @alpha
 */
export const removeListenerAction = createAction(
  'actionListenerMiddleware/remove',
  function prepare(
    typeOrActionCreator: string | TypedActionCreator<string>,
    listener: ActionListener<any, any, any, any>
  ) {
    const type =
      typeof typeOrActionCreator === 'string'
        ? typeOrActionCreator
        : (typeOrActionCreator as TypedActionCreator<string>).type;

    return {
      payload: {
        type,
        listener
      }
    };
  }
) as BaseActionCreator<
  { type: string; listener: ActionListener<any, any, any, any> },
  'actionListenerMiddleware/remove'
> & {
  <C extends TypedActionCreator<any>, S, D extends Dispatch>(
    actionCreator: C,
    listener: ActionListener<ReturnType<C>, S, D, any>
  ): RemoveListenerAction<ReturnType<C>, S, D>;

  <S, D extends Dispatch>(type: string, listener: ActionListener<AnyAction, S, D, any>): RemoveListenerAction<
    AnyAction,
    S,
    D
  >;
};

/**
 * @alpha
 */
export function createActionListenerMiddleware<S, D extends Dispatch<AnyAction> = Dispatch>() {
  type ListenerEntry = ActionListenerOptions & {
    listener: ActionListener<any, S, D, any>;
    unsubscribe: () => void;
  };

  type ListenerEntryWithMatcher = ListenerEntry & {
    matcher: MatchFunction<any>;
  };

  const listenerMap: Record<string, Set<ListenerEntry> | undefined> = {};
  const matcherListeners = new Set<ListenerEntryWithMatcher>();

  const middleware: Middleware<
    {
      (action: Action<'actionListenerMiddleware/add'>): Unsubscribe;
    },
    S,
    D
  > = api => next => action => {
    if (addListenerAction.match(action)) {
      const unsubscribe = addListener(action.payload.type, action.payload.listener, action.payload.options);

      return unsubscribe;
    }
    if (removeListenerAction.match(action)) {
      // @ts-ignore
      removeListener(action.payload.type, action.payload.listener);

      return;
    }
    // @ts-ignore
    const listeners = listenerMap[action.type];
    let matchedMatcherListeners: ListenerEntry[] = [];

    if (matcherListeners.size > 0) {
      matchedMatcherListeners = Array.from(matcherListeners).filter(entry => {
        return entry.matcher(action);
      });
    }

    if (listeners || matcherListeners.size > 0) {
      const allListeners = Array.from(listeners ?? []).concat(matchedMatcherListeners);
      const defaultWhen = 'after';
      let result: unknown;
      for (const phase of ['before', 'after'] as const) {
        for (const entry of allListeners) {
          if (phase !== (entry.when || defaultWhen)) {
            continue;
          }
          let stoppedPropagation = false;
          let currentPhase = phase;
          let synchronousListenerFinished = false;
          entry.listener(action, {
            ...api,
            stopPropagation() {
              if (currentPhase === 'before') {
                if (!synchronousListenerFinished) {
                  stoppedPropagation = true;
                } else {
                  throw new Error('stopPropagation can only be called synchronously');
                }
              } else {
                throw new Error(
                  'stopPropagation can only be called by action listeners with the `when` option set to "before"'
                );
              }
            },
            unsubscribe: entry.unsubscribe
          });
          synchronousListenerFinished = true;
          if (stoppedPropagation) {
            return action;
          }
        }
        if (phase === 'before') {
          result = next(action);
        } else {
          return result;
        }
      }
    }
    return next(action);
  };

  type Unsubscribe = () => void;

  function addStringListener<T extends string, O extends ActionListenerOptions>(
    type: T,
    listener: ActionListener<Action<T>, S, D, O>,
    options?: O
  ): Unsubscribe {
    const listeners = getListenerMap(type);
    let entry = findListenerEntry(listeners, listener);

    if (!entry) {
      entry = {
        ...options,
        listener,
        unsubscribe: () => listeners.delete(entry!)
      };

      listeners.add(entry);
    }

    return entry.unsubscribe;
  }

  function addMatcherListener<MA extends AnyAction, M extends MatchFunction<MA>, O extends ActionListenerOptions>(
    matcher: M,
    listener: ActionListener<MA, S, D, O>,
    options?: O
  ): Unsubscribe {
    let entry = findListenerEntry(matcherListeners, listener) as ListenerEntryWithMatcher | undefined;

    if (!entry) {
      entry = {
        ...options,
        listener,
        matcher,
        unsubscribe: () => matcherListeners.delete(entry!)
      };

      matcherListeners.add(entry);
    }

    return entry.unsubscribe;
  }

  type GuardedType<T> = T extends (x: any) => x is infer T ? T : never;

  function addListener<C extends TypedActionCreator<any>, O extends ActionListenerOptions>(
    actionCreator: C,
    listener: ActionListener<ReturnType<C>, S, D, O>,
    options?: O
  ): Unsubscribe;
  // eslint-disable-next-line no-redeclare
  function addListener<T extends string, O extends ActionListenerOptions>(
    type: T,
    listener: ActionListener<Action<T>, S, D, O>,
    options?: O
  ): Unsubscribe;
  // eslint-disable-next-line no-redeclare
  function addListener<MA extends AnyAction, M extends MatchFunction<MA>, O extends ActionListenerOptions>(
    matcher: M,
    listener: ActionListener<GuardedType<M>, S, D, O>,
    options?: O
  ): Unsubscribe;
  // eslint-disable-next-line no-redeclare
  function addListener(
    typeOrActionCreator: string | TypedActionCreator<any>,
    listener: ActionListener<AnyAction, S, D, any>,
    options?: ActionListenerOptions
  ): Unsubscribe {
    if (typeof typeOrActionCreator === 'string') {
      return addStringListener(typeOrActionCreator, listener, options);
    } else if (typeof typeOrActionCreator.type === 'string') {
      return addStringListener(typeOrActionCreator.type, listener, options);
    } else {
      const matcher = typeOrActionCreator as unknown as MatchFunction<any>;
      return addMatcherListener(matcher, listener, options);
    }
  }

  function getListenerMap(type: string) {
    if (!listenerMap[type]) {
      listenerMap[type] = new Set();
    }
    return listenerMap[type]!;
  }

  function removeListener<C extends TypedActionCreator<any>>(
    actionCreator: C,
    listener: ActionListener<ReturnType<C>, S, D, any>
  ): boolean;
  // eslint-disable-next-line no-redeclare
  function removeListener(type: string, listener: ActionListener<AnyAction, S, D, any>): boolean;
  // eslint-disable-next-line no-redeclare
  function removeListener(
    typeOrActionCreator: string | TypedActionCreator<any>,
    listener: ActionListener<AnyAction, S, D, any>
  ): boolean {
    const type = typeof typeOrActionCreator === 'string' ? typeOrActionCreator : typeOrActionCreator.type;

    const listeners = listenerMap[type];

    if (!listeners) {
      return false;
    }

    let entry = findListenerEntry(listeners, listener);

    if (!entry) {
      return false;
    }

    listeners.delete(entry);
    return true;
  }

  function findListenerEntry(entries: Set<ListenerEntry>, listener: Function): ListenerEntry | undefined {
    for (const entry of entries) {
      if (entry.listener === listener) {
        return entry;
      }
    }
  }

  return Object.assign(middleware, { addListener, removeListener }, {} as WithMiddlewareType<typeof middleware>);
}

[markerikson]

I forgot to mention it earlier, but I did publish a standalone version of this middleware as https://www.npmjs.com/package/@rtk-incubator/action-listener-middleware a few days ago. We'd appreciate feedback on whether this API is working out!

[markerikson]

I just collected a list of relevant existing discussions and comments over in the corresponding Discussions thread:

#1648 (comment)

I'm going to go through this again tomorrow and extract a list of bullet points for things we ought to nail down specifically.

[markerikson]

I've gone through the issue/PR threads for our WIP "action listener middleware", collected a list of outstanding API design questions that need to be solved, and summarized them along with my own suggestions:

#1648 (reply in thread)

Please give us feedback on these questions!

[markerikson]

This PR is stale now that a copy of the middleware has been merged into the repo and published as a standalone @rtk-incubator/action-listener-middleware package, so I'm going to close this.

I just published v0.2.0, which cleans up the API based on numerous suggestions, and also adds a condition util to support long-running async workflows!

Please see the discussion in #1648 for a change log of the temporary package and provide us feedback!