Merge pull request #2000 from reduxjs/fix-serializableCheck

reduxjs · Feb 16, 2022 · eff7527 · eff7527
2 parents 563f705 + 0dea2e7
commit eff7527
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 31 deletions.
diff --git a/packages/toolkit/src/serializableStateInvariantMiddleware.ts b/packages/toolkit/src/serializableStateInvariantMiddleware.ts
@@ -169,41 +169,40 @@ export function createSerializableStateInvariantMiddleware(
   } = options
 
   return (storeAPI) => (next) => (action) => {
-    if (
-      ignoreActions ||
-      (ignoredActions.length && ignoredActions.indexOf(action.type) !== -1)
-    ) {
-      return next(action)
-    }
+    const result = next(action)
 
     const measureUtils = getTimeMeasureUtils(
       warnAfter,
       'SerializableStateInvariantMiddleware'
     )
-    measureUtils.measureTime(() => {
-      const foundActionNonSerializableValue = findNonSerializableValue(
-        action,
-        '',
-        isSerializable,
-        getEntries,
-        ignoredActionPaths
-      )
-
-      if (foundActionNonSerializableValue) {
-        const { keyPath, value } = foundActionNonSerializableValue
 
-        console.error(
-          `A non-serializable value was detected in an action, in the path: \`${keyPath}\`. Value:`,
-          value,
-          '\nTake a look at the logic that dispatched this action: ',
+    if (
+      !ignoreActions &&
+      !(ignoredActions.length && ignoredActions.indexOf(action.type) !== -1)
+    ) {
+      measureUtils.measureTime(() => {
+        const foundActionNonSerializableValue = findNonSerializableValue(
           action,
-          '\n(See https://redux.js.org/faq/actions#why-should-type-be-a-string-or-at-least-serializable-why-should-my-action-types-be-constants)',
-          '\n(To allow non-serializable values see: https://redux-toolkit.js.org/usage/usage-guide#working-with-non-serializable-data)'
+          '',
+          isSerializable,
+          getEntries,
+          ignoredActionPaths
         )
-      }
-    })
 
-    const result = next(action)
+        if (foundActionNonSerializableValue) {
+          const { keyPath, value } = foundActionNonSerializableValue
+
+          console.error(
+            `A non-serializable value was detected in an action, in the path: \`${keyPath}\`. Value:`,
+            value,
+            '\nTake a look at the logic that dispatched this action: ',
+            action,
+            '\n(See https://redux.js.org/faq/actions#why-should-type-be-a-string-or-at-least-serializable-why-should-my-action-types-be-constants)',
+            '\n(To allow non-serializable values see: https://redux-toolkit.js.org/usage/usage-guide#working-with-non-serializable-data)'
+          )
+        }
+      })
+    }
 
     if (!ignoreState) {
       measureUtils.measureTime(() => {

diff --git a/packages/toolkit/src/tests/serializableStateInvariantMiddleware.test.ts b/packages/toolkit/src/tests/serializableStateInvariantMiddleware.test.ts
@@ -326,11 +326,13 @@ describe('serializableStateInvariantMiddleware', () => {
 
     store.dispatch({ type: 'IGNORE_ME' })
 
-    expect(numTimesCalled).toBe(0)
+    // The state check only calls `isSerializable` once
+    expect(numTimesCalled).toBe(1)
 
     store.dispatch({ type: 'ANY_OTHER_ACTION' })
 
-    expect(numTimesCalled).toBeGreaterThan(0)
+    // Action checks call `isSerializable` 2+ times when enabled
+    expect(numTimesCalled).toBeGreaterThanOrEqual(3)
   })
 
   describe('ignored action paths', () => {
@@ -410,7 +412,12 @@ describe('serializableStateInvariantMiddleware', () => {
 
     store.dispatch({ type: 'THIS_DOESNT_MATTER' })
 
-    expect(numTimesCalled).toBe(0)
+    // `isSerializable` is called once for a state check
+    expect(numTimesCalled).toBe(1)
+
+    store.dispatch({ type: 'THIS_DOESNT_MATTER_AGAIN' })
+
+    expect(numTimesCalled).toBe(2)
   })
 
   it('should not check serializability for ignored slice names', () => {
@@ -470,17 +477,55 @@ describe('serializableStateInvariantMiddleware', () => {
 
   it('allows ignoring state entirely', () => {
     const badValue = new Map()
+    let numTimesCalled = 0
     const reducer = () => badValue
-    configureStore({
+    const store = configureStore({
       reducer,
       middleware: [
         createSerializableStateInvariantMiddleware({
+          isSerializable: () => {
+            numTimesCalled++
+            return true
+          },
           ignoreState: true,
         }),
       ],
-    }).dispatch({ type: 'test' })
+    })
+
+    expect(numTimesCalled).toBe(0)
+
+    store.dispatch({ type: 'test' })
 
     expect(getLog().log).toMatchInlineSnapshot(`""`)
+
+    // Should be called twice for the action - there is an initial check for early returns, then a second and potentially 3rd for nested properties
+    expect(numTimesCalled).toBe(2)
+  })
+
+  it('never calls isSerializable if both ignoreState and ignoreActions are true', () => {
+    const badValue = new Map()
+    let numTimesCalled = 0
+    const reducer = () => badValue
+    const store = configureStore({
+      reducer,
+      middleware: [
+        createSerializableStateInvariantMiddleware({
+          isSerializable: () => {
+            numTimesCalled++
+            return true
+          },
+          ignoreState: true,
+          ignoreActions: true,
+        }),
+      ],
+    })
+
+    expect(numTimesCalled).toBe(0)
+
+    store.dispatch({ type: 'TEST', payload: new Date() })
+    store.dispatch({ type: 'OTHER_THING' })
+
+    expect(numTimesCalled).toBe(0)
   })
 
   it('Should print a warning if execution takes too long', () => {