Merge pull request reduxjs#1851 from reduxjs/feature/listener-remove-…

…when
brandoncroberts · Dec 20, 2021 · 7d138a0 · 7d138a0
2 parents 9a232a8 + 4280a27
commit 7d138a0
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 207 deletions.
diff --git a/packages/action-listener-middleware/README.md b/packages/action-listener-middleware/README.md
@@ -143,8 +143,6 @@ interface AddListenerOptions {
   predicate?: ListenerPredicate
 
   listener: (action: Action, listenerApi: ListenerApi) => void | Promise<void>
-
-  when?: 'beforeReducer' | 'afterReducer' | 'both'
 }
 ```
 
@@ -174,15 +172,7 @@ The return value is a standard `unsubscribe()` callback that will remove this li
 
 The `listener` callback will receive the current action as its first argument, as well as a "listener API" object similar to the "thunk API" object in `createAsyncThunk`.
 
-The listener may be configured to run _before_ an action reaches the reducer, _after_ the reducer, or both, by passing a `when` option when adding the listener. If the `when` option is not provided, the default is 'afterReducer':
-
-```ts
-middleware.addListener({
-  actionCreator: increment,
-  listener,
-  when: 'beforeReducer',
-})
-```
+All listener predicates and callbacks are checked _after_ the root reducer has already processed the action and updated the state. The `listenerApi.getOriginalState()` method can be used to get the state value that existed before the action that triggered this listener was processed.
 
 ### `listenerMiddleware.removeListener(typeOrActionCreator, listener)`
 
@@ -226,7 +216,6 @@ The `listenerApi` object is the second argument to each listener callback. It co
 
 #### Middleware Options
 
-- `currentPhase: 'beforeReducer' | 'afterReducer'`: an string indicating when the listener is being called relative to the action processing
 - `extra: unknown`: the "extra argument" that was provided as part of the middleware setup, if any
 
 `extra` can be used to inject a value such as an API service layer into the middleware at creation time, and is accessible here.

diff --git a/packages/action-listener-middleware/src/index.ts b/packages/action-listener-middleware/src/index.ts
@@ -25,7 +25,6 @@ import type {
   ListenerEntry,
   ListenerErrorHandler,
   Unsubscribe,
-  MiddlewarePhase,
   WithMiddlewareType,
   TakePattern,
   ListenerErrorInfo,
@@ -48,8 +47,6 @@ export type {
   ActionListenerMiddlewareAPI,
   ActionListenerOptions,
   CreateListenerMiddlewareOptions,
-  MiddlewarePhase,
-  When,
   ListenerErrorHandler,
   TypedAddListener,
   TypedAddListenerAction,
@@ -68,11 +65,7 @@ export type {
 //Overly-aggressive byte-shaving
 const { assign } = Object
 
-const beforeReducer = 'beforeReducer' as const
-const afterReducer = 'afterReducer' as const
-
-const defaultWhen: MiddlewarePhase = afterReducer
-const actualMiddlewarePhases = [beforeReducer, afterReducer] as const
+const alm = 'actionListenerMiddleware' as const
 
 const createFork = (parentAbortSignal: AbortSignal) => {
   return <T>(taskExecutor: ForkedTaskExecutor<T>): ForkedTask<T> => {
@@ -184,18 +177,17 @@ export const createListenerEntry: TypedCreateListenerEntry<unknown> = (
     // pass
   } else {
     throw new Error(
-      'Creating a listener requires one of the known fields for matching against actions'
+      'Creating a listener requires one of the known fields for matching an action'
     )
   }
 
   const id = nanoid()
   const entry: ListenerEntry<unknown> = {
-    when: options.when || defaultWhen,
     id,
     listener,
     type,
     predicate,
-    pendingSet: new Set<AbortController>(),
+    pending: new Set<AbortController>(),
     unsubscribe: () => {
       throw new Error('Unsubscribe not initialized')
     },
@@ -231,7 +223,7 @@ const safelyNotifyError = (
  * @alpha
  */
 export const addListenerAction = createAction(
-  'actionListenerMiddleware/add',
+  `${alm}/add`,
   function prepare(options: unknown) {
     const entry = createListenerEntry(
       // Fake out TS here
@@ -248,7 +240,7 @@ export const addListenerAction = createAction(
  * @alpha
  */
 export const removeListenerAction = createAction(
-  'actionListenerMiddleware/remove',
+  `${alm}/remove`,
   function prepare(
     typeOrActionCreator: string | TypedActionCreator<string>,
     listener: ActionListener<any, any, any>
@@ -281,7 +273,7 @@ export const removeListenerAction = createAction(
 }
 
 const defaultErrorHandler: ListenerErrorHandler = (...args: unknown[]) => {
-  console.error('action-listener-middleware-error', ...args)
+  console.error(`${alm}/error`, ...args)
 }
 
 /**
@@ -362,12 +354,14 @@ export function createActionListenerMiddleware<
     entry: ListenerEntry<unknown, Dispatch<AnyAction>>,
     action: AnyAction,
     api: MiddlewareAPI,
-    getOriginalState: () => S,
-    currentPhase: MiddlewarePhase
+    getOriginalState: () => S
   ) => {
     const internalTaskController = new AbortController()
     const take = createTakePattern(addListener, internalTaskController.signal)
-    const condition: ConditionFunction<S> = (predicate, timeout) => {
+    const condition: ConditionFunction<S> = (
+      predicate: AnyActionListenerPredicate<any>,
+      timeout?: number
+    ) => {
       return take(predicate, timeout).then(Boolean)
     }
     const delay = createDelay(internalTaskController.signal)
@@ -376,7 +370,7 @@ export function createActionListenerMiddleware<
       internalTaskController.signal
     )
     try {
-      entry.pendingSet.add(internalTaskController)
+      entry.pending.add(internalTaskController)
       await Promise.resolve(
         entry.listener(
           action,
@@ -387,7 +381,6 @@ export function createActionListenerMiddleware<
             take,
             delay,
             pause,
-            currentPhase,
             extra,
             signal: internalTaskController.signal,
             fork,
@@ -396,7 +389,7 @@ export function createActionListenerMiddleware<
               listenerMap.set(entry.id, entry)
             },
             cancelPrevious: () => {
-              entry.pendingSet.forEach((controller, _, set) => {
+              entry.pending.forEach((controller, _, set) => {
                 if (controller !== internalTaskController) {
                   controller.abort()
                   set.delete(controller)
@@ -410,18 +403,17 @@ export function createActionListenerMiddleware<
       if (!(listenerError instanceof TaskAbortError)) {
         safelyNotifyError(onError, listenerError, {
           raisedBy: 'listener',
-          phase: currentPhase,
         })
       }
     } finally {
       internalTaskController.abort() // Notify that the task has completed
-      entry.pendingSet.delete(internalTaskController)
+      entry.pending.delete(internalTaskController)
     }
   }
 
   const middleware: Middleware<
     {
-      (action: Action<'actionListenerMiddleware/add'>): Unsubscribe
+      (action: Action<`${typeof alm}/add`>): Unsubscribe
     },
     S,
     D
@@ -442,47 +434,37 @@ export function createActionListenerMiddleware<
       return
     }
 
-    if (listenerMap.size === 0) {
-      return next(action)
-    }
-
-    let result: unknown
+    // Need to get this state _before_ the reducer processes the action
     const originalState = api.getState()
     const getOriginalState = () => originalState
 
-    for (const currentPhase of actualMiddlewarePhases) {
+    // Actually forward the action to the reducer before we handle listeners
+    const result: unknown = next(action)
+
+    if (listenerMap.size > 0) {
       let currentState = api.getState()
       for (let entry of listenerMap.values()) {
-        const runThisPhase =
-          entry.when === 'both' || entry.when === currentPhase
-
-        let runListener = runThisPhase
-
-        if (runListener) {
-          try {
-            runListener = entry.predicate(action, currentState, originalState)
-          } catch (predicateError) {
-            runListener = false
-
-            safelyNotifyError(onError, predicateError, {
-              raisedBy: 'predicate',
-              phase: currentPhase,
-            })
-          }
+        let runListener = false
+
+        try {
+          runListener = entry.predicate(action, currentState, originalState)
+        } catch (predicateError) {
+          runListener = false
+
+          safelyNotifyError(onError, predicateError, {
+            raisedBy: 'predicate',
+          })
         }
 
         if (!runListener) {
           continue
         }
 
-        notifyListener(entry, action, api, getOriginalState, currentPhase)
-      }
-      if (currentPhase === beforeReducer) {
-        result = next(action)
-      } else {
-        return result
+        notifyListener(entry, action, api, getOriginalState)
       }
     }
+
+    return result
   }
 
   return assign(

diff --git a/packages/action-listener-middleware/src/tests/effectScenarios.test.ts b/packages/action-listener-middleware/src/tests/effectScenarios.test.ts
@@ -7,21 +7,9 @@ import {
 
 import type { AnyAction, PayloadAction, Action } from '@reduxjs/toolkit'
 
-import {
-  createActionListenerMiddleware,
-  createListenerEntry,
-  addListenerAction,
-  removeListenerAction,
-  TaskAbortError,
-} from '../index'
-
-import type {
-  When,
-  ActionListenerMiddlewareAPI,
-  TypedAddListenerAction,
-  TypedAddListener,
-  Unsubscribe,
-} from '../index'
+import { createActionListenerMiddleware, TaskAbortError } from '../index'
+
+import type { TypedAddListener } from '../index'
 
 describe('Saga-style Effects Scenarios', () => {
   interface CounterState {