Merge pull request #660 from Methuselah96/improve-identity-function-c…

…heck
reduxjs · Dec 17, 2023 · 0837509 · 0837509
2 parents cf8d96b + 997534f
commit 0837509
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 30 deletions.
diff --git a/src/createSelectorCreator.ts b/src/createSelectorCreator.ts
@@ -364,12 +364,18 @@ export function createSelectorCreator<
         arguments
       )
 
+      // apply arguments instead of spreading for performance.
+      // @ts-ignore
+      lastResult = memoizedResultFunc.apply(null, inputSelectorResults)
+
       if (process.env.NODE_ENV !== 'production') {
         const { identityFunctionCheck, inputStabilityCheck } =
           getDevModeChecksExecutionInfo(firstRun, devModeChecks)
         if (identityFunctionCheck.shouldRun) {
           identityFunctionCheck.run(
-            resultFunc as Combiner<InputSelectors, Result>
+            resultFunc as Combiner<InputSelectors, Result>,
+            inputSelectorResults,
+            lastResult
           )
         }
 
@@ -390,10 +396,6 @@ export function createSelectorCreator<
         if (firstRun) firstRun = false
       }
 
-      // apply arguments instead of spreading for performance.
-      // @ts-ignore
-      lastResult = memoizedResultFunc.apply(null, inputSelectorResults)
-
       return lastResult
     }, ...finalArgsMemoizeOptions) as unknown as Selector<
       GetStateFromSelectors<InputSelectors>,

diff --git a/src/devModeChecks/identityFunctionCheck.ts b/src/devModeChecks/identityFunctionCheck.ts
@@ -9,34 +9,45 @@ import type { AnyFunction } from '../types'
  * extraction logic in input selectors.
  *
  * @param resultFunc - The result function to be checked.
+ * @param inputSelectorsResults - The results of the input selectors.
+ * @param outputSelectorResult - The result of the output selector.
  *
  * @see {@link https://reselect.js.org/api/development-only-stability-checks#identityfunctioncheck `identityFunctionCheck`}
  *
  * @since 5.0.0
  * @internal
  */
-export const runIdentityFunctionCheck = (resultFunc: AnyFunction) => {
-  let isInputSameAsOutput = false
-  try {
-    const emptyObject = {}
-    if (resultFunc(emptyObject) === emptyObject) isInputSameAsOutput = true
-  } catch {
-    // Do nothing
-  }
-  if (isInputSameAsOutput) {
-    let stack: string | undefined = undefined
+export const runIdentityFunctionCheck = (
+  resultFunc: AnyFunction,
+  inputSelectorsResults: unknown[],
+  outputSelectorResult: unknown
+) => {
+  if (
+    inputSelectorsResults.length === 1 &&
+    inputSelectorsResults[0] === outputSelectorResult
+  ) {
+    let isInputSameAsOutput = false
     try {
-      throw new Error()
-    } catch (e) {
-      // eslint-disable-next-line @typescript-eslint/no-extra-semi, no-extra-semi
-      ;({ stack } = e as Error)
+      const emptyObject = {}
+      if (resultFunc(emptyObject) === emptyObject) isInputSameAsOutput = true
+    } catch {
+      // Do nothing
+    }
+    if (isInputSameAsOutput) {
+      let stack: string | undefined = undefined
+      try {
+        throw new Error()
+      } catch (e) {
+        // eslint-disable-next-line @typescript-eslint/no-extra-semi, no-extra-semi
+        ;({ stack } = e as Error)
+      }
+      console.warn(
+        'The result function returned its own inputs without modification. e.g' +
+          '\n`createSelector([state => state.todos], todos => todos)`' +
+          '\nThis could lead to inefficient memoization and unnecessary re-renders.' +
+          '\nEnsure transformation logic is in the result function, and extraction logic is in the input selectors.',
+        { stack }
+      )
     }
-    console.warn(
-      'The result function returned its own inputs without modification. e.g' +
-        '\n`createSelector([state => state.todos], todos => todos)`' +
-        '\nThis could lead to inefficient memoization and unnecessary re-renders.' +
-        '\nEnsure transformation logic is in the result function, and extraction logic is in the input selectors.',
-      { stack }
-    )
   }
 }
diff --git a/test/identityFunctionCheck.test.ts b/test/identityFunctionCheck.test.ts
@@ -181,4 +181,56 @@ describe('identityFunctionCheck', () => {
 
     expect(consoleSpy).toHaveBeenCalledOnce()
   })
+
+  localTest(
+    'does not warn if result function is not identity function (case 1)',
+    ({ state }) => {
+      // This test demonstrates why in some cases it can be useful to compare the first argument of the result
+      // function with the returned value (and not just checking for an identity function by passing `{}` to the result
+      // function).
+      const getFirstAlertIfMessageIsEmpty = createSelector(
+        [(state: RootState) => state.alerts[0]],
+        firstAlert => (!firstAlert.message ? firstAlert : null)
+      )
+
+      expect(getFirstAlertIfMessageIsEmpty(state)).toBeNull()
+
+      expect(consoleSpy).not.toHaveBeenCalled()
+    }
+  )
+
+  localTest(
+    'does not warn if result function is not identity function (case 2)',
+    ({ state }) => {
+      // This test demonstrates why in some cases it can be useful to pass `{}` into the result function and compare it
+      // with the returned value (and not just checking for an identity function by passing the first argument to the
+      // result function).
+      const getFirstAlertIfMessageIsNotEmpty = createSelector(
+        [(state: RootState) => state.alerts[0]],
+        firstAlert => (firstAlert.message ? firstAlert : null)
+      )
+
+      expect(getFirstAlertIfMessageIsNotEmpty(state)).toBe(state.alerts[0])
+
+      expect(consoleSpy).not.toHaveBeenCalled()
+    }
+  )
+
+  localTest(
+    'does not warn if result function is passed more than one argument',
+    ({ state }) => {
+      const getAllNotificationsIfSmsNotEnabled = createSelector(
+        [
+          (state: RootState) => state.alerts,
+          (state: RootState) =>
+            state.users.user.details.preferences.notifications.sms
+        ],
+        (alerts, smsEnabled) => (!smsEnabled ? alerts : [])
+      )
+
+      expect(getAllNotificationsIfSmsNotEnabled(state)).toBe(state.alerts)
+
+      expect(consoleSpy).not.toHaveBeenCalled()
+    }
+  )
 })
diff --git a/test/lruMemoize.test.ts b/test/lruMemoize.test.ts
@@ -274,12 +274,12 @@ describe(lruMemoize, () => {
     )
 
     fooChangeHandler(state)
-    expect(fooChangeSpy.mock.calls.length).toEqual(2)
+    expect(fooChangeSpy.mock.calls.length).toEqual(1)
 
     // no change
     fooChangeHandler(state)
     // this would fail
-    expect(fooChangeSpy.mock.calls.length).toEqual(2)
+    expect(fooChangeSpy.mock.calls.length).toEqual(1)
 
     const state2 = { a: 1 }
     let count = 0

diff --git a/test/reselect.spec.ts b/test/reselect.spec.ts
@@ -215,7 +215,7 @@ describe('Basic selector behavior', () => {
     )
     expect(() => selector({ a: 1 })).toThrow('test error')
     expect(() => selector({ a: 1 })).toThrow('test error')
-    expect(called).toBe(3)
+    expect(called).toBe(2)
   })
 
   test('memoizes previous result before exception', () => {

diff --git a/test/weakmapMemoize.spec.ts b/test/weakmapMemoize.spec.ts
@@ -148,7 +148,7 @@ describe('Basic selector behavior with weakMapMemoize', () => {
     )
     expect(() => selector({ a: 1 })).toThrow('test error')
     expect(() => selector({ a: 1 })).toThrow('test error')
-    expect(called).toBe(3)
+    expect(called).toBe(2)
   })
 
   test('memoizes previous result before exception', () => {