From 69959aeb26be90bc2cf6a3a0877a09c7ef22564c Mon Sep 17 00:00:00 2001 From: Nathan Bierema Date: Sun, 10 Dec 2023 16:57:13 -0500 Subject: [PATCH 1/5] Update identityFunctionCheck to check number of arguments --- src/createSelectorCreator.ts | 31 +++++++++---- src/devModeChecks/identityFunctionCheck.ts | 53 +++++++++++++--------- 2 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/createSelectorCreator.ts b/src/createSelectorCreator.ts index 1581e7fda..e5edfa473 100644 --- a/src/createSelectorCreator.ts +++ b/src/createSelectorCreator.ts @@ -365,13 +365,10 @@ export function createSelectorCreator< ) if (process.env.NODE_ENV !== 'production') { - const { identityFunctionCheck, inputStabilityCheck } = - getDevModeChecksExecutionInfo(firstRun, devModeChecks) - if (identityFunctionCheck.shouldRun) { - identityFunctionCheck.run( - resultFunc as Combiner - ) - } + const { inputStabilityCheck } = getDevModeChecksExecutionInfo( + firstRun, + devModeChecks + ) if (inputStabilityCheck.shouldRun) { // make a second copy of the params, to check if we got the same results @@ -386,14 +383,30 @@ export function createSelectorCreator< arguments ) } - - if (firstRun) firstRun = false } // apply arguments instead of spreading for performance. // @ts-ignore lastResult = memoizedResultFunc.apply(null, inputSelectorResults) + if (process.env.NODE_ENV !== 'production') { + const { identityFunctionCheck } = getDevModeChecksExecutionInfo( + firstRun, + devModeChecks + ) + if (identityFunctionCheck.shouldRun) { + identityFunctionCheck.run( + resultFunc as Combiner, + inputSelectorResults, + lastResult + ) + } + } + + if (process.env.NODE_ENV !== 'production') { + if (firstRun) firstRun = false + } + 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 } - ) } } From 43043f9e7989fcf38ad638e0eab5d49b88445fe6 Mon Sep 17 00:00:00 2001 From: Nathan Bierema Date: Sun, 10 Dec 2023 16:57:22 -0500 Subject: [PATCH 2/5] Update tests --- test/lruMemoize.test.ts | 4 ++-- test/reselect.spec.ts | 2 +- test/weakmapMemoize.spec.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) 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', () => { From 058c4799194e84bb506c775d0b7400855680f402 Mon Sep 17 00:00:00 2001 From: Nathan Bierema Date: Sun, 10 Dec 2023 17:14:37 -0500 Subject: [PATCH 3/5] Add some tests --- test/identityFunctionCheck.test.ts | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/identityFunctionCheck.test.ts b/test/identityFunctionCheck.test.ts index eee26f61f..506b09c1e 100644 --- a/test/identityFunctionCheck.test.ts +++ b/test/identityFunctionCheck.test.ts @@ -181,4 +181,38 @@ 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() + } + ) }) From 97c0e5716f3e90549c381fded141cba13bb813d0 Mon Sep 17 00:00:00 2001 From: Nathan Bierema Date: Sun, 10 Dec 2023 17:25:05 -0500 Subject: [PATCH 4/5] Add another test --- test/identityFunctionCheck.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/identityFunctionCheck.test.ts b/test/identityFunctionCheck.test.ts index 506b09c1e..fa7fd225e 100644 --- a/test/identityFunctionCheck.test.ts +++ b/test/identityFunctionCheck.test.ts @@ -215,4 +215,22 @@ describe('identityFunctionCheck', () => { 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() + } + ) }) From 997534f71c52f72dec96f75073779b26d18c5af4 Mon Sep 17 00:00:00 2001 From: Nathan Bierema Date: Sun, 10 Dec 2023 17:58:37 -0500 Subject: [PATCH 5/5] Move dev checks back into single block To get the performance tests to pass (?!) --- src/createSelectorCreator.ts | 37 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/createSelectorCreator.ts b/src/createSelectorCreator.ts index e5edfa473..c5b7fec5b 100644 --- a/src/createSelectorCreator.ts +++ b/src/createSelectorCreator.ts @@ -364,11 +364,20 @@ 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 { inputStabilityCheck } = getDevModeChecksExecutionInfo( - firstRun, - devModeChecks - ) + const { identityFunctionCheck, inputStabilityCheck } = + getDevModeChecksExecutionInfo(firstRun, devModeChecks) + if (identityFunctionCheck.shouldRun) { + identityFunctionCheck.run( + resultFunc as Combiner, + inputSelectorResults, + lastResult + ) + } if (inputStabilityCheck.shouldRun) { // make a second copy of the params, to check if we got the same results @@ -383,27 +392,7 @@ 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 } = getDevModeChecksExecutionInfo( - firstRun, - devModeChecks - ) - if (identityFunctionCheck.shouldRun) { - identityFunctionCheck.run( - resultFunc as Combiner, - inputSelectorResults, - lastResult - ) - } - } - - if (process.env.NODE_ENV !== 'production') { if (firstRun) firstRun = false }