diff --git a/src/createSelectorCreator.ts b/src/createSelectorCreator.ts index 1581e7fda..c5b7fec5b 100644 --- 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 + resultFunc as Combiner, + 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, diff --git a/src/devModeChecks/identityFunctionCheck.ts b/src/devModeChecks/identityFunctionCheck.ts index ae005d216..487a8ece5 100644 --- 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 index eee26f61f..fa7fd225e 100644 --- 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 index cf04e8cc1..5a6583959 100644 --- 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 index 047dfb8bf..69734ba71 100644 --- 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 index 8a68e3e17..f59e37600 100644 --- 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', () => {