Merge pull request #1902 from FaberVitale/fix/alm-getOriginalState-leaky-behavior

Author: markerikson

packages/action-listener-middleware/README.md

@@ -210,7 +210,7 @@ The `listenerApi` object is the second argument to each listener callback. It co
 
 - `dispatch: Dispatch`: the standard `store.dispatch` method
 - `getState: () => State`: the standard `store.getState` method
-- `getOriginalState: () => State`: returns the store state as it existed when the action was originally dispatched, _before_ the reducers ran
+- `getOriginalState: () => State`: returns the store state as it existed when the action was originally dispatched, _before_ the reducers ran. (**Note**: this method can only be called synchronously, during the initial dispatch call stack, to avoid memory leaks. Calling it asynchronously will throw an error.)
 
 `dispatch` and `getState` are exactly the same as in a thunk. `getOriginalState` can be used to compare the original state before the listener was started.
 

packages/action-listener-middleware/src/index.ts

@@ -64,6 +64,10 @@ export type {
 
 //Overly-aggressive byte-shaving
 const { assign } = Object
+/**
+ * @internal
+ */
+const INTERNAL_NIL_TOKEN = {} as const
 
 const alm = 'actionListenerMiddleware' as const
 
@@ -432,33 +436,51 @@ export function createActionListenerMiddleware<
     }
 
     // Need to get this state _before_ the reducer processes the action
-    const originalState = api.getState()
-    const getOriginalState = () => originalState
+    let originalState: S | typeof INTERNAL_NIL_TOKEN = api.getState()
+
+    // `getOriginalState` can only be called synchronously.
+    // @see https://github.com/reduxjs/redux-toolkit/discussions/1648#discussioncomment-1932820
+    const getOriginalState = (): S => {
+      if (originalState === INTERNAL_NIL_TOKEN) {
+        throw new Error(
+          `${alm}: getOriginalState can only be called synchronously`
+        )
+      }
 
-    // Actually forward the action to the reducer before we handle listeners
-    const result: unknown = next(action)
+      return originalState as S
+    }
 
-    if (listenerMap.size > 0) {
-      let currentState = api.getState()
-      for (let entry of listenerMap.values()) {
-        let runListener = false
+    let result: unknown
 
-        try {
-          runListener = entry.predicate(action, currentState, originalState)
-        } catch (predicateError) {
-          runListener = false
+    try {
+      // Actually forward the action to the reducer before we handle listeners
+      result = next(action)
 
-          safelyNotifyError(onError, predicateError, {
-            raisedBy: 'predicate',
-          })
-        }
+      if (listenerMap.size > 0) {
+        let currentState = api.getState()
+        for (let entry of listenerMap.values()) {
+          let runListener = false
 
-        if (!runListener) {
-          continue
-        }
+          try {
+            runListener = entry.predicate(action, currentState, originalState)
+          } catch (predicateError) {
+            runListener = false
+
+            safelyNotifyError(onError, predicateError, {
+              raisedBy: 'predicate',
+            })
+          }
 
-        notifyListener(entry, action, api, getOriginalState)
+          if (!runListener) {
+            continue
+          }
+
+          notifyListener(entry, action, api, getOriginalState)
+        }
       }
+    } finally {
+      // Remove `originalState` store from this scope.
+      originalState = INTERNAL_NIL_TOKEN
     }
 
     return result

packages/action-listener-middleware/src/tests/listenerMiddleware.test.ts

@@ -21,6 +21,7 @@ import type {
   TypedAddListenerAction,
   TypedAddListener,
   Unsubscribe,
+  ActionListenerMiddleware,
 } from '../index'
 
 const middlewareApi = {
@@ -531,6 +532,56 @@ describe('createActionListenerMiddleware', () => {
       expect(listener2Calls).toBe(1)
     })
 
+    test('getOriginalState can only be invoked synchronously', async () => {
+      const onError = jest.fn()
+
+      middleware = createActionListenerMiddleware({ onError })
+      const store = configureStore({
+        reducer: counterSlice.reducer,
+        middleware: (gDM) => gDM().prepend(middleware),
+      })
+
+      let appMidleware = middleware as ActionListenerMiddleware<
+        CounterState,
+        typeof store.dispatch
+      >
+
+      appMidleware.addListener({
+        actionCreator: increment,
+        async listener(_, listenerApi) {
+          const runIncrementBy = () => {
+            listenerApi.dispatch(
+              counterSlice.actions.incrementByAmount(
+                listenerApi.getOriginalState().value + 2
+              )
+            )
+          }
+
+          runIncrementBy()
+
+          await Promise.resolve()
+
+          runIncrementBy()
+        },
+      })
+
+      expect(store.getState()).toEqual({ value: 0 })
+
+      store.dispatch(increment()) // state.value+=1 && trigger listener
+      expect(onError).not.toHaveBeenCalled()
+      expect(store.getState()).toEqual({ value: 3 })
+
+      await delay(0)
+
+      expect(onError).toBeCalledWith(
+        new Error(
+          'actionListenerMiddleware: getOriginalState can only be called synchronously'
+        ),
+        { raisedBy: 'listener' }
+      )
+      expect(store.getState()).toEqual({ value: 3 })
+    })
+
     test('by default, actions are forwarded to the store', () => {
       reducer.mockClear()
 

packages/action-listener-middleware/src/types.ts

@@ -118,6 +118,30 @@ export interface ForkedTask<T> {
  */
 export interface ActionListenerMiddlewareAPI<S, D extends Dispatch<AnyAction>>
   extends MiddlewareAPI<D, S> {
+  /**
+   * Returns the store state as it existed when the action was originally dispatched, _before_ the reducers ran.
+   *
+   * ### Synchronous invocation
+   *
+   * This function can **only** be invoked **synchronously**, it throws error otherwise.
+   *
+   * @example
+   *
+   * ```ts
+   * middleware.addListener({
+   *  predicate: () => true,
+   *  async listener(_, { getOriginalState }) {
+   *    getOriginalState(); // sync: OK!
+   *
+   *    setTimeout(getOriginalState, 0); // async: throws Error
+   *
+   *    await Promise().resolve();
+   *
+   *    getOriginalState() // async: throws Error
+   *  }
+   * })
+   * ```
+   */
   getOriginalState: () => S
   unsubscribe(): void
   subscribe(): void