From 74331cb56a9915a3a9166053c4d66b2f72187b9b Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sat, 13 May 2023 15:22:37 +0100 Subject: [PATCH 01/12] Begin investigating running input selectors twice, to check stability. --- src/index.ts | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index 66711e0f9..b22f48bf5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -26,6 +26,7 @@ export type { } from './types' import { + createCacheKeyComparator, defaultMemoize, defaultEqualityCheck, DefaultMemoizeOptions @@ -98,7 +99,10 @@ export function createSelectorCreator< // Determine which set of options we're using. Prefer options passed directly, // but fall back to options given to createSelectorCreator. - const { memoizeOptions = memoizeOptionsFromArgs } = directlyPassedOptions + const { + memoizeOptions = memoizeOptionsFromArgs, + inputStabilityCheck = true + } = directlyPassedOptions // Simplifying assumption: it's unlikely that the first options arg of the provided memoizer // is an array. In most libs I've looked at, it's an equality function or options object. @@ -111,6 +115,9 @@ export function createSelectorCreator< const dependencies = getDependencies(funcs) + // do we want to try and use the equality fn from options? how do we account for other memoizers? + const cacheKeyComparator = createCacheKeyComparator(defaultEqualityCheck) + const memoizedResultFunc = memoize( function recomputationWrapper() { recomputations++ @@ -133,10 +140,32 @@ export function createSelectorCreator< const params = [] const length = dependencies.length + const paramsCopy = [] + for (let i = 0; i < length; i++) { // apply arguments instead of spreading and mutate a local list of params for performance. // @ts-ignore params.push(dependencies[i].apply(null, arguments)) + + if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) { + // make a second copy of the params, to check if we got the same results + // @ts-ignore + paramsCopy.push(dependencies[i].apply(null, arguments)) + } + } + + if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) { + const equal = cacheKeyComparator(params, paramsCopy) + if (!equal) { + // do we want to log more information about the selector? + console.warn( + `An input selector returned a different result when passed same arguments. + This means your output selector will likely run more frequently than intended. + Avoid returning a new reference inside your input selector, e.g. + \`createSelector([(arg1, arg2) => ({ arg1, arg2 })],(arg1, arg2) => {})\` + ` + ) + } } // apply arguments instead of spreading for performance. @@ -164,7 +193,8 @@ export function createSelectorCreator< } export interface CreateSelectorOptions { - memoizeOptions: MemoizeOptions[0] | MemoizeOptions + memoizeOptions?: MemoizeOptions[0] | MemoizeOptions + inputStabilityCheck?: boolean } /** From ecfb455b3c1e1a92c9760d15dca89e799078297b Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sat, 13 May 2023 17:36:11 +0100 Subject: [PATCH 02/12] move entire check inside single if block --- src/index.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index b22f48bf5..abf085436 100644 --- a/src/index.ts +++ b/src/index.ts @@ -138,23 +138,22 @@ export function createSelectorCreator< // TODO Rethink this change, or find a way to expose more options? const selector = defaultMemoize(function dependenciesChecker() { const params = [] - const length = dependencies.length - - const paramsCopy = [] + const { length } = dependencies for (let i = 0; i < length; i++) { // apply arguments instead of spreading and mutate a local list of params for performance. // @ts-ignore params.push(dependencies[i].apply(null, arguments)) + } + + if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) { + const paramsCopy = [] - if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) { + for (let i = 0; i < length; i++) { // make a second copy of the params, to check if we got the same results // @ts-ignore paramsCopy.push(dependencies[i].apply(null, arguments)) } - } - - if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) { const equal = cacheKeyComparator(params, paramsCopy) if (!equal) { // do we want to log more information about the selector? From 643175365cd5787750694803a541b9de677fe819 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sat, 13 May 2023 20:00:57 +0100 Subject: [PATCH 03/12] run performance checks with production settings --- test/autotrackMemoize.spec.ts | 87 +++++++++++++++++++--------------- test/perfComparisons.spec.ts | 9 ++++ test/reselect.spec.ts | 88 +++++++++++++++++++---------------- 3 files changed, 107 insertions(+), 77 deletions(-) diff --git a/test/autotrackMemoize.spec.ts b/test/autotrackMemoize.spec.ts index be6db56fe..d1edc7be4 100644 --- a/test/autotrackMemoize.spec.ts +++ b/test/autotrackMemoize.spec.ts @@ -92,52 +92,63 @@ describe('Basic selector behavior with autotrack', () => { ) }) - test('basic selector cache hit performance', () => { - if (process.env.COVERAGE) { - return // don't run performance tests for coverage - } + describe('performance checks', () => { + const originalEnv = process.env.NODE_ENV + + beforeAll(() => { + process.env.NODE_ENV = 'production' + }) + afterAll(() => { + process.env.NODE_NV = originalEnv + }) + + test('basic selector cache hit performance', () => { + if (process.env.COVERAGE) { + return // don't run performance tests for coverage + } - const selector = createSelector( - (state: StateAB) => state.a, - (state: StateAB) => state.b, - (a, b) => a + b - ) - const state1 = { a: 1, b: 2 } + const selector = createSelector( + (state: StateAB) => state.a, + (state: StateAB) => state.b, + (a, b) => a + b + ) + const state1 = { a: 1, b: 2 } - const start = performance.now() - for (let i = 0; i < 1000000; i++) { - selector(state1) - } - const totalTime = performance.now() - start + const start = performance.now() + for (let i = 0; i < 1000000; i++) { + selector(state1) + } + const totalTime = performance.now() - start - expect(selector(state1)).toBe(3) - expect(selector.recomputations()).toBe(1) - // Expected a million calls to a selector with the same arguments to take less than 1 second - expect(totalTime).toBeLessThan(1000) - }) + expect(selector(state1)).toBe(3) + expect(selector.recomputations()).toBe(1) + // Expected a million calls to a selector with the same arguments to take less than 1 second + expect(totalTime).toBeLessThan(1000) + }) - test('basic selector cache hit performance for state changes but shallowly equal selector args', () => { - if (process.env.COVERAGE) { - return // don't run performance tests for coverage - } + test('basic selector cache hit performance for state changes but shallowly equal selector args', () => { + if (process.env.COVERAGE) { + return // don't run performance tests for coverage + } - const selector = createSelector( - (state: StateAB) => state.a, - (state: StateAB) => state.b, - (a, b) => a + b - ) + const selector = createSelector( + (state: StateAB) => state.a, + (state: StateAB) => state.b, + (a, b) => a + b + ) - const start = performance.now() - for (let i = 0; i < 1000000; i++) { - selector(states[i]) - } - const totalTime = performance.now() - start + const start = performance.now() + for (let i = 0; i < 1000000; i++) { + selector(states[i]) + } + const totalTime = performance.now() - start - expect(selector(states[0])).toBe(3) - expect(selector.recomputations()).toBe(1) + expect(selector(states[0])).toBe(3) + expect(selector.recomputations()).toBe(1) - // Expected a million calls to a selector with the same arguments to take less than 1 second - expect(totalTime).toBeLessThan(1000) + // Expected a million calls to a selector with the same arguments to take less than 1 second + expect(totalTime).toBeLessThan(1000) + }) }) test('memoized composite arguments', () => { diff --git a/test/perfComparisons.spec.ts b/test/perfComparisons.spec.ts index 4e50cb276..17e5bd917 100644 --- a/test/perfComparisons.spec.ts +++ b/test/perfComparisons.spec.ts @@ -8,6 +8,15 @@ import { vi } from 'vitest' import { configureStore, createSlice, PayloadAction } from '@reduxjs/toolkit' describe('More perf comparisons', () => { + const originalEnv = process.env.NODE_ENV + + beforeAll(() => { + process.env.NODE_ENV = 'production' + }) + afterAll(() => { + process.env.NODE_NV = originalEnv + }) + const csDefault = createSelectorCreator(defaultMemoize) const csAutotrack = createSelectorCreator(autotrackMemoize) diff --git a/test/reselect.spec.ts b/test/reselect.spec.ts index a201b2115..47c4c00ce 100644 --- a/test/reselect.spec.ts +++ b/test/reselect.spec.ts @@ -101,54 +101,64 @@ describe('Basic selector behavior', () => { ) }) - test('basic selector cache hit performance', () => { - if (process.env.COVERAGE) { - return // don't run performance tests for coverage - } + describe('performance checks', () => { + const originalEnv = process.env.NODE_ENV + + beforeAll(() => { + process.env.NODE_ENV = 'production' + }) + afterAll(() => { + process.env.NODE_NV = originalEnv + }) + + test('basic selector cache hit performance', () => { + if (process.env.COVERAGE) { + return // don't run performance tests for coverage + } - const selector = createSelector( - (state: StateAB) => state.a, - (state: StateAB) => state.b, - (a, b) => a + b - ) - const state1 = { a: 1, b: 2 } + const selector = createSelector( + (state: StateAB) => state.a, + (state: StateAB) => state.b, + (a, b) => a + b + ) + const state1 = { a: 1, b: 2 } - const start = performance.now() - for (let i = 0; i < 1000000; i++) { - selector(state1) - } - const totalTime = performance.now() - start + const start = performance.now() + for (let i = 0; i < 1000000; i++) { + selector(state1) + } + const totalTime = performance.now() - start - expect(selector(state1)).toBe(3) - expect(selector.recomputations()).toBe(1) - // Expected a million calls to a selector with the same arguments to take less than 1 second - expect(totalTime).toBeLessThan(1000) - }) + expect(selector(state1)).toBe(3) + expect(selector.recomputations()).toBe(1) + // Expected a million calls to a selector with the same arguments to take less than 1 second + expect(totalTime).toBeLessThan(1000) + }) - test('basic selector cache hit performance for state changes but shallowly equal selector args', () => { - if (process.env.COVERAGE) { - return // don't run performance tests for coverage - } + test('basic selector cache hit performance for state changes but shallowly equal selector args', () => { + if (process.env.COVERAGE) { + return // don't run performance tests for coverage + } - const selector = createSelector( - (state: StateAB) => state.a, - (state: StateAB) => state.b, - (a, b) => a + b - ) + const selector = createSelector( + (state: StateAB) => state.a, + (state: StateAB) => state.b, + (a, b) => a + b + ) - const start = new Date() - for (let i = 0; i < numOfStates; i++) { - selector(states[i]) - } - const totalTime = new Date().getTime() - start.getTime() + const start = new Date() + for (let i = 0; i < numOfStates; i++) { + selector(states[i]) + } + const totalTime = new Date().getTime() - start.getTime() - expect(selector(states[0])).toBe(3) - expect(selector.recomputations()).toBe(1) + expect(selector(states[0])).toBe(3) + expect(selector.recomputations()).toBe(1) - // Expected a million calls to a selector with the same arguments to take less than 1 second - expect(totalTime).toBeLessThan(1000) + // Expected a million calls to a selector with the same arguments to take less than 1 second + expect(totalTime).toBeLessThan(1000) + }) }) - test('memoized composite arguments', () => { const selector = createSelector( (state: StateSub) => state.sub, From 903b46354fdfc4586159434769379b5fe2809a35 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sat, 13 May 2023 22:01:25 +0100 Subject: [PATCH 04/12] add some tests --- src/index.ts | 4 +- test/inputStabilityCheck.spec.ts | 63 ++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 test/inputStabilityCheck.spec.ts diff --git a/src/index.ts b/src/index.ts index abf085436..23ea1dea6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -77,9 +77,7 @@ export function createSelectorCreator< // Due to the intricacies of rest params, we can't do an optional arg after `...funcs`. // So, start by declaring the default value here. // (And yes, the words 'memoize' and 'options' appear too many times in this next sequence.) - let directlyPassedOptions: CreateSelectorOptions = { - memoizeOptions: undefined - } + let directlyPassedOptions: CreateSelectorOptions = {} // Normally, the result func or "output selector" is the last arg let resultFunc = funcs.pop() diff --git a/test/inputStabilityCheck.spec.ts b/test/inputStabilityCheck.spec.ts new file mode 100644 index 000000000..e5e70bc71 --- /dev/null +++ b/test/inputStabilityCheck.spec.ts @@ -0,0 +1,63 @@ +import { createSelector } from 'reselect' + +describe('inputStabilityCheck', () => { + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + const unstableInput = vi.fn((a: number, b: number) => ({ a, b })) + afterEach(() => { + consoleSpy.mockClear() + unstableInput.mockClear() + }) + afterAll(() => { + consoleSpy.mockRestore() + }) + + it('calls each input selector twice, and warns to console if unstable reference is returned', () => { + const stableAddNums = createSelector( + [(a: number) => a, (a, b: number) => b], + (a, b) => a + b + ) + + expect(stableAddNums(1, 2)).toBe(3) + + expect(consoleSpy).not.toHaveBeenCalled() + + const addNums = createSelector([unstableInput], ({ a, b }) => a + b) + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(2) + + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('An input selector returned a different result') + ) + }) + + it('disables check if specified', () => { + const addNums = createSelector([unstableInput], ({ a, b }) => a + b, { + inputStabilityCheck: false + }) + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(1) + + expect(consoleSpy).not.toHaveBeenCalled() + }) + + it('disables check in production', () => { + const originalEnv = process.env.NODE_ENV + + process.env.NODE_ENV = 'production' + + const addNums = createSelector([unstableInput], ({ a, b }) => a + b) + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(1) + + expect(consoleSpy).not.toHaveBeenCalled() + + process.env.NODE_ENV = originalEnv + }) +}) From f1f6fef3aa8add78026adc759d5d0e9ba1126273 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sat, 13 May 2023 23:52:41 +0100 Subject: [PATCH 05/12] use a library-wide flag instead of per-selector config --- src/index.ts | 23 +++++++++++++++-------- test/inputStabilityCheck.spec.ts | 18 +++++++++--------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/index.ts b/src/index.ts index 23ea1dea6..461e9d581 100644 --- a/src/index.ts +++ b/src/index.ts @@ -39,6 +39,12 @@ export { defaultMemoize, defaultEqualityCheck } export type { DefaultMemoizeOptions } +let inputStabilityCheckDisabled = false + +export function disableInputStabilityCheck(disable = true) { + inputStabilityCheckDisabled = disable +} + function getDependencies(funcs: unknown[]) { const dependencies = Array.isArray(funcs[0]) ? funcs[0] : funcs @@ -77,7 +83,9 @@ export function createSelectorCreator< // Due to the intricacies of rest params, we can't do an optional arg after `...funcs`. // So, start by declaring the default value here. // (And yes, the words 'memoize' and 'options' appear too many times in this next sequence.) - let directlyPassedOptions: CreateSelectorOptions = {} + let directlyPassedOptions: CreateSelectorOptions = { + memoizeOptions: undefined + } // Normally, the result func or "output selector" is the last arg let resultFunc = funcs.pop() @@ -97,10 +105,7 @@ export function createSelectorCreator< // Determine which set of options we're using. Prefer options passed directly, // but fall back to options given to createSelectorCreator. - const { - memoizeOptions = memoizeOptionsFromArgs, - inputStabilityCheck = true - } = directlyPassedOptions + const { memoizeOptions = memoizeOptionsFromArgs } = directlyPassedOptions // Simplifying assumption: it's unlikely that the first options arg of the provided memoizer // is an array. In most libs I've looked at, it's an equality function or options object. @@ -144,7 +149,10 @@ export function createSelectorCreator< params.push(dependencies[i].apply(null, arguments)) } - if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) { + if ( + process.env.NODE_ENV !== 'production' && + !inputStabilityCheckDisabled + ) { const paramsCopy = [] for (let i = 0; i < length; i++) { @@ -190,8 +198,7 @@ export function createSelectorCreator< } export interface CreateSelectorOptions { - memoizeOptions?: MemoizeOptions[0] | MemoizeOptions - inputStabilityCheck?: boolean + memoizeOptions: MemoizeOptions[0] | MemoizeOptions } /** diff --git a/test/inputStabilityCheck.spec.ts b/test/inputStabilityCheck.spec.ts index e5e70bc71..db26f152a 100644 --- a/test/inputStabilityCheck.spec.ts +++ b/test/inputStabilityCheck.spec.ts @@ -1,12 +1,16 @@ -import { createSelector } from 'reselect' +import { createSelector, disableInputStabilityCheck } from 'reselect' describe('inputStabilityCheck', () => { const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) const unstableInput = vi.fn((a: number, b: number) => ({ a, b })) + + const addNums = createSelector([unstableInput], ({ a, b }) => a + b) + afterEach(() => { consoleSpy.mockClear() unstableInput.mockClear() + addNums.clearCache() }) afterAll(() => { consoleSpy.mockRestore() @@ -14,7 +18,7 @@ describe('inputStabilityCheck', () => { it('calls each input selector twice, and warns to console if unstable reference is returned', () => { const stableAddNums = createSelector( - [(a: number) => a, (a, b: number) => b], + [(a: number) => a, (a: number, b: number) => b], (a, b) => a + b ) @@ -22,8 +26,6 @@ describe('inputStabilityCheck', () => { expect(consoleSpy).not.toHaveBeenCalled() - const addNums = createSelector([unstableInput], ({ a, b }) => a + b) - expect(addNums(1, 2)).toBe(3) expect(unstableInput).toHaveBeenCalledTimes(2) @@ -34,15 +36,15 @@ describe('inputStabilityCheck', () => { }) it('disables check if specified', () => { - const addNums = createSelector([unstableInput], ({ a, b }) => a + b, { - inputStabilityCheck: false - }) + disableInputStabilityCheck() expect(addNums(1, 2)).toBe(3) expect(unstableInput).toHaveBeenCalledTimes(1) expect(consoleSpy).not.toHaveBeenCalled() + + disableInputStabilityCheck(false) }) it('disables check in production', () => { @@ -50,8 +52,6 @@ describe('inputStabilityCheck', () => { process.env.NODE_ENV = 'production' - const addNums = createSelector([unstableInput], ({ a, b }) => a + b) - expect(addNums(1, 2)).toBe(3) expect(unstableInput).toHaveBeenCalledTimes(1) From db2c62193c7f742c1806a84564ac587fa7ba6667 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sun, 14 May 2023 00:00:52 +0100 Subject: [PATCH 06/12] more intuitive naming --- src/index.ts | 8 ++++---- test/inputStabilityCheck.spec.ts | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index 461e9d581..d69f0fd1b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -39,10 +39,10 @@ export { defaultMemoize, defaultEqualityCheck } export type { DefaultMemoizeOptions } -let inputStabilityCheckDisabled = false +let inputStabilityCheckEnabled = true -export function disableInputStabilityCheck(disable = true) { - inputStabilityCheckDisabled = disable +export function setInputStabilityCheckEnabled(enabled: boolean) { + inputStabilityCheckEnabled = enabled } function getDependencies(funcs: unknown[]) { @@ -151,7 +151,7 @@ export function createSelectorCreator< if ( process.env.NODE_ENV !== 'production' && - !inputStabilityCheckDisabled + !inputStabilityCheckEnabled ) { const paramsCopy = [] diff --git a/test/inputStabilityCheck.spec.ts b/test/inputStabilityCheck.spec.ts index db26f152a..5d4086fd1 100644 --- a/test/inputStabilityCheck.spec.ts +++ b/test/inputStabilityCheck.spec.ts @@ -1,4 +1,4 @@ -import { createSelector, disableInputStabilityCheck } from 'reselect' +import { createSelector, setInputStabilityCheckEnabled } from 'reselect' describe('inputStabilityCheck', () => { const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -36,7 +36,7 @@ describe('inputStabilityCheck', () => { }) it('disables check if specified', () => { - disableInputStabilityCheck() + setInputStabilityCheckEnabled(false) expect(addNums(1, 2)).toBe(3) @@ -44,7 +44,7 @@ describe('inputStabilityCheck', () => { expect(consoleSpy).not.toHaveBeenCalled() - disableInputStabilityCheck(false) + setInputStabilityCheckEnabled(true) }) it('disables check in production', () => { From e4102b6ffe4699eadb4eff8c3381600749a112aa Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sun, 14 May 2023 00:56:22 +0100 Subject: [PATCH 07/12] use provided memoize for stability check (and fix flag) --- package.json | 2 ++ src/index.ts | 20 +++++++++++-------- test/inputStabilityCheck.spec.ts | 19 ++++++++++++++++++ yarn.lock | 34 +++++++++++++++++++++++++++++++- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index 40c4b06cb..424670f6f 100644 --- a/package.json +++ b/package.json @@ -67,6 +67,8 @@ "memoize-one": "^6.0.0", "micro-memoize": "^4.0.9", "prettier": "^2.7.1", + "react": "^18.2.0", + "react-dom": "^18.2.0", "react-redux": "^8.0.2", "rimraf": "^3.0.2", "shelljs": "^0.8.5", diff --git a/src/index.ts b/src/index.ts index d69f0fd1b..6823cae18 100644 --- a/src/index.ts +++ b/src/index.ts @@ -26,7 +26,6 @@ export type { } from './types' import { - createCacheKeyComparator, defaultMemoize, defaultEqualityCheck, DefaultMemoizeOptions @@ -118,8 +117,7 @@ export function createSelectorCreator< const dependencies = getDependencies(funcs) - // do we want to try and use the equality fn from options? how do we account for other memoizers? - const cacheKeyComparator = createCacheKeyComparator(defaultEqualityCheck) + console.log(process.env.NODE_ENV, !inputStabilityCheckEnabled) const memoizedResultFunc = memoize( function recomputationWrapper() { @@ -130,6 +128,13 @@ export function createSelectorCreator< ...finalMemoizeOptions ) + // @ts-ignore + const makeAnObject: (...args: unknown[]) => object = memoize( + // @ts-ignore + () => ({}), + ...finalMemoizeOptions + ) + // If a selector is called with the exact same arguments we don't need to traverse our dependencies again. // TODO This was changed to `memoize` in 4.0.0 ( #297 ), but I changed it back. // The original intent was to allow customizing things like skortchmark's @@ -149,10 +154,7 @@ export function createSelectorCreator< params.push(dependencies[i].apply(null, arguments)) } - if ( - process.env.NODE_ENV !== 'production' && - !inputStabilityCheckEnabled - ) { + if (process.env.NODE_ENV !== 'production' && inputStabilityCheckEnabled) { const paramsCopy = [] for (let i = 0; i < length; i++) { @@ -160,7 +162,9 @@ export function createSelectorCreator< // @ts-ignore paramsCopy.push(dependencies[i].apply(null, arguments)) } - const equal = cacheKeyComparator(params, paramsCopy) + const equal = + makeAnObject.apply(null, params) === + makeAnObject.apply(null, paramsCopy) if (!equal) { // do we want to log more information about the selector? console.warn( diff --git a/test/inputStabilityCheck.spec.ts b/test/inputStabilityCheck.spec.ts index 5d4086fd1..b93dca97a 100644 --- a/test/inputStabilityCheck.spec.ts +++ b/test/inputStabilityCheck.spec.ts @@ -1,4 +1,5 @@ import { createSelector, setInputStabilityCheckEnabled } from 'reselect' +import { shallowEqual } from 'react-redux' describe('inputStabilityCheck', () => { const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -60,4 +61,22 @@ describe('inputStabilityCheck', () => { process.env.NODE_ENV = originalEnv }) + + it('uses the memoize provided', () => { + const addNumsShallow = createSelector( + [unstableInput], + ({ a, b }) => a + b, + { + memoizeOptions: { + equalityCheck: shallowEqual + } + } + ) + + expect(addNumsShallow(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(2) + + expect(consoleSpy).not.toHaveBeenCalled() + }) }) diff --git a/yarn.lock b/yarn.lock index c7ac583a8..b6ee3bb84 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2281,7 +2281,7 @@ __metadata: languageName: node linkType: hard -"loose-envify@npm:^1.4.0": +"loose-envify@npm:^1.1.0, loose-envify@npm:^1.4.0": version: 1.4.0 resolution: "loose-envify@npm:1.4.0" dependencies: @@ -2923,6 +2923,18 @@ __metadata: languageName: node linkType: hard +"react-dom@npm:^18.2.0": + version: 18.2.0 + resolution: "react-dom@npm:18.2.0" + dependencies: + loose-envify: ^1.1.0 + scheduler: ^0.23.0 + peerDependencies: + react: ^18.2.0 + checksum: 7d323310bea3a91be2965f9468d552f201b1c27891e45ddc2d6b8f717680c95a75ae0bc1e3f5cf41472446a2589a75aed4483aee8169287909fcd59ad149e8cc + languageName: node + linkType: hard + "react-is@npm:^16.7.0, react-is@npm:^16.8.1": version: 16.13.1 resolution: "react-is@npm:16.13.1" @@ -2976,6 +2988,15 @@ __metadata: languageName: node linkType: hard +"react@npm:^18.2.0": + version: 18.2.0 + resolution: "react@npm:18.2.0" + dependencies: + loose-envify: ^1.1.0 + checksum: 88e38092da8839b830cda6feef2e8505dec8ace60579e46aa5490fc3dc9bba0bd50336507dc166f43e3afc1c42939c09fe33b25fae889d6f402721dcd78fca1b + languageName: node + linkType: hard + "readable-stream@npm:^2.0.6": version: 2.3.7 resolution: "readable-stream@npm:2.3.7" @@ -3089,6 +3110,8 @@ __metadata: memoize-one: ^6.0.0 micro-memoize: ^4.0.9 prettier: ^2.7.1 + react: ^18.2.0 + react-dom: ^18.2.0 react-redux: ^8.0.2 rimraf: ^3.0.2 shelljs: ^0.8.5 @@ -3220,6 +3243,15 @@ __metadata: languageName: node linkType: hard +"scheduler@npm:^0.23.0": + version: 0.23.0 + resolution: "scheduler@npm:0.23.0" + dependencies: + loose-envify: ^1.1.0 + checksum: d79192eeaa12abef860c195ea45d37cbf2bbf5f66e3c4dcd16f54a7da53b17788a70d109ee3d3dde1a0fd50e6a8fc171f4300356c5aee4fc0171de526bf35f8a + languageName: node + linkType: hard + "semver@npm:^6.3.0": version: 6.3.0 resolution: "semver@npm:6.3.0" From ebcde3ceeb8ead18a9989654e1dc56af82fea1a2 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sun, 14 May 2023 11:49:50 +0100 Subject: [PATCH 08/12] add per selector option back, and support running check once --- src/index.ts | 44 ++++++++++++++++++++------------ test/inputStabilityCheck.spec.ts | 32 ++++++++++++++++++++++- 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/src/index.ts b/src/index.ts index 6823cae18..eb89b2507 100644 --- a/src/index.ts +++ b/src/index.ts @@ -38,10 +38,12 @@ export { defaultMemoize, defaultEqualityCheck } export type { DefaultMemoizeOptions } -let inputStabilityCheckEnabled = true +type StabilityCheck = boolean | 'once' -export function setInputStabilityCheckEnabled(enabled: boolean) { - inputStabilityCheckEnabled = enabled +let globalStabilityCheck: StabilityCheck = true + +export function setInputStabilityCheckEnabled(enabled: StabilityCheck) { + globalStabilityCheck = enabled } function getDependencies(funcs: unknown[]) { @@ -82,9 +84,7 @@ export function createSelectorCreator< // Due to the intricacies of rest params, we can't do an optional arg after `...funcs`. // So, start by declaring the default value here. // (And yes, the words 'memoize' and 'options' appear too many times in this next sequence.) - let directlyPassedOptions: CreateSelectorOptions = { - memoizeOptions: undefined - } + let directlyPassedOptions: CreateSelectorOptions = {} // Normally, the result func or "output selector" is the last arg let resultFunc = funcs.pop() @@ -104,7 +104,8 @@ export function createSelectorCreator< // Determine which set of options we're using. Prefer options passed directly, // but fall back to options given to createSelectorCreator. - const { memoizeOptions = memoizeOptionsFromArgs } = directlyPassedOptions + const { memoizeOptions = memoizeOptionsFromArgs, inputStabilityCheck } = + directlyPassedOptions // Simplifying assumption: it's unlikely that the first options arg of the provided memoizer // is an array. In most libs I've looked at, it's an equality function or options object. @@ -117,8 +118,6 @@ export function createSelectorCreator< const dependencies = getDependencies(funcs) - console.log(process.env.NODE_ENV, !inputStabilityCheckEnabled) - const memoizedResultFunc = memoize( function recomputationWrapper() { recomputations++ @@ -135,6 +134,8 @@ export function createSelectorCreator< ...finalMemoizeOptions ) + let firstRun = true + // If a selector is called with the exact same arguments we don't need to traverse our dependencies again. // TODO This was changed to `memoize` in 4.0.0 ( #297 ), but I changed it back. // The original intent was to allow customizing things like skortchmark's @@ -154,7 +155,13 @@ export function createSelectorCreator< params.push(dependencies[i].apply(null, arguments)) } - if (process.env.NODE_ENV !== 'production' && inputStabilityCheckEnabled) { + const finalStabilityCheck = inputStabilityCheck ?? globalStabilityCheck + + if ( + process.env.NODE_ENV !== 'production' && + (finalStabilityCheck === true || + (finalStabilityCheck === 'once' && firstRun)) + ) { const paramsCopy = [] for (let i = 0; i < length; i++) { @@ -162,23 +169,27 @@ export function createSelectorCreator< // @ts-ignore paramsCopy.push(dependencies[i].apply(null, arguments)) } + + // if the memoize method thinks the parameters are equal, these *should* be the same reference const equal = makeAnObject.apply(null, params) === makeAnObject.apply(null, paramsCopy) if (!equal) { // do we want to log more information about the selector? console.warn( - `An input selector returned a different result when passed same arguments. - This means your output selector will likely run more frequently than intended. - Avoid returning a new reference inside your input selector, e.g. - \`createSelector([(arg1, arg2) => ({ arg1, arg2 })],(arg1, arg2) => {})\` - ` + 'An input selector returned a different result when passed same arguments.' + + '\nThis means your output selector will likely run more frequently than intended.' + + '\nAvoid returning a new reference inside your input selector, e.g.' + + '\n`createSelector([(arg1, arg2) => ({ arg1, arg2 })],(arg1, arg2) => {})`' ) } + + if (firstRun) firstRun = false } // apply arguments instead of spreading for performance. lastResult = memoizedResultFunc.apply(null, params) + return lastResult } as F) @@ -202,7 +213,8 @@ export function createSelectorCreator< } export interface CreateSelectorOptions { - memoizeOptions: MemoizeOptions[0] | MemoizeOptions + memoizeOptions?: MemoizeOptions[0] | MemoizeOptions + inputStabilityCheck?: StabilityCheck } /** diff --git a/test/inputStabilityCheck.spec.ts b/test/inputStabilityCheck.spec.ts index b93dca97a..5c3e005c4 100644 --- a/test/inputStabilityCheck.spec.ts +++ b/test/inputStabilityCheck.spec.ts @@ -36,7 +36,7 @@ describe('inputStabilityCheck', () => { ) }) - it('disables check if specified', () => { + it('disables check if global setting is changed', () => { setInputStabilityCheckEnabled(false) expect(addNums(1, 2)).toBe(3) @@ -48,6 +48,18 @@ describe('inputStabilityCheck', () => { setInputStabilityCheckEnabled(true) }) + it('disables check if specified in the selector options', () => { + const addNums = createSelector([unstableInput], ({ a, b }) => a + b, { + inputStabilityCheck: false + }) + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(1) + + expect(consoleSpy).not.toHaveBeenCalled() + }) + it('disables check in production', () => { const originalEnv = process.env.NODE_ENV @@ -62,6 +74,24 @@ describe('inputStabilityCheck', () => { process.env.NODE_ENV = originalEnv }) + it('allows running the check only once', () => { + const addNums = createSelector([unstableInput], ({ a, b }) => a + b, { + inputStabilityCheck: 'once' + }) + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(2) + + expect(consoleSpy).toHaveBeenCalledOnce() + + expect(addNums(2, 2)).toBe(4) + + expect(unstableInput).toHaveBeenCalledTimes(3) + + expect(consoleSpy).toHaveBeenCalledOnce() + }) + it('uses the memoize provided', () => { const addNumsShallow = createSelector( [unstableInput], From 1e597af01caa38ed712ff044b7297d4867e45504 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sun, 14 May 2023 14:30:45 +0100 Subject: [PATCH 09/12] default check to "once" instead of always --- src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.ts b/src/index.ts index eb89b2507..9ca945865 100644 --- a/src/index.ts +++ b/src/index.ts @@ -40,7 +40,7 @@ export type { DefaultMemoizeOptions } type StabilityCheck = boolean | 'once' -let globalStabilityCheck: StabilityCheck = true +let globalStabilityCheck: StabilityCheck = 'once' export function setInputStabilityCheckEnabled(enabled: StabilityCheck) { globalStabilityCheck = enabled From eb7b5fd4d8f664836571e4c890942216d0b5ad76 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sun, 14 May 2023 17:09:39 +0100 Subject: [PATCH 10/12] add to README --- README.md | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/README.md b/README.md index 14f4454de..670e20341 100644 --- a/README.md +++ b/README.md @@ -309,6 +309,66 @@ const nestedSelector = createStructuredSelector({ }) ``` +## Development-only checks + +### `inputStabilityCheck` + +In development, an extra check is conducted on your input selectors. It runs your input selectors an extra time with the same parameters, and warns in console if they return a different result (based on your `memoize` method). + +This is important, as an input selector returning a materially different result with the same parameters means that the output selector will be run unnecessarily, thus (potentially) creating a new result and causing rerenders. + +```js +const addNumbers = createSelector( + // this input selector will always return a new reference when run + // so cache will never be used + (a, b) => ({ a, b }), + ({ a, b }) => ({ total: a + b }) +) +// instead, you should have an input selector for each stable piece of data +const addNumbersStable = createSelector( + (a, b) => a, + (a, b) => b, + (a, b) => ({ + total: a + b + }) +) +``` + +By default, this will only happen when the selector is first called. You can configure the check globally or per selector, to change it to always run when the selector is called, or to never run. + +_This check is disabled for production environments._ + +#### Global configuration + +A `setInputStabilityCheckEnabled` function is exported from `reselect`, which should be called with the desired setting. + +```js +import { setInputStabilityCheckEnabled } from 'reselect' + +// run when selector is first called (default) +setInputStabilityCheckEnabled('once') + +// always run +setInputStabilityCheckEnabled(true) + +// never run +setInputStabilityCheckEnabled(false) +``` + +#### Per-selector configuration + +A value can be passed as part of the selector options object, which will override the global setting for the given selector. + +```ts +const selectPersonName = createSelector( + selectPerson, + person => person.firstName + ' ' + person.lastName, + // `inputStabilityCheck` accepts the same settings + // as `setInputStabilityCheckEnabled` + { inputStabilityCheck: false } +) +``` + ## FAQ ### Q: Why isn’t my selector recomputing when the input state changes? From a163264341ab043584501add83d091a753b6adc2 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sun, 14 May 2023 17:13:24 +0100 Subject: [PATCH 11/12] update table of contents --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 670e20341..b87dc8bda 100644 --- a/README.md +++ b/README.md @@ -88,6 +88,10 @@ console.log(selectTotal(exampleState)) // { total: 2.322 } - [Customize `equalityCheck` for `defaultMemoize`](#customize-equalitycheck-for-defaultmemoize) - [Use memoize function from Lodash for an unbounded cache](#use-memoize-function-from-lodash-for-an-unbounded-cache) - [createStructuredSelector({inputSelectors}, selectorCreator = createSelector)](#createstructuredselectorinputselectors-selectorcreator--createselector) +- [Development-only checks](#development-only-checks) + - [`inputStabilityCheck`](#inputstabilitycheck) + - [Global configuration](#global-configuration) + - [Per-selector configuration](#per-selector-configuration) - [FAQ](#faq) - [Q: Why isn’t my selector recomputing when the input state changes?](#q-why-isnt-my-selector-recomputing-when-the-input-state-changes) - [Q: Why is my selector recomputing when the input state stays the same?](#q-why-is-my-selector-recomputing-when-the-input-state-stays-the-same) From afc8bd34b91a0f7e625ff31e19083b12f4412fd8 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Sun, 14 May 2023 22:38:30 +0100 Subject: [PATCH 12/12] use an enum, and log arguments and inputs --- src/index.ts | 11 ++++++++--- test/inputStabilityCheck.spec.ts | 18 ++++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index 9ca945865..66067705c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -38,7 +38,7 @@ export { defaultMemoize, defaultEqualityCheck } export type { DefaultMemoizeOptions } -type StabilityCheck = boolean | 'once' +type StabilityCheck = 'always' | 'once' | 'never' let globalStabilityCheck: StabilityCheck = 'once' @@ -159,7 +159,7 @@ export function createSelectorCreator< if ( process.env.NODE_ENV !== 'production' && - (finalStabilityCheck === true || + (finalStabilityCheck === 'always' || (finalStabilityCheck === 'once' && firstRun)) ) { const paramsCopy = [] @@ -180,7 +180,12 @@ export function createSelectorCreator< 'An input selector returned a different result when passed same arguments.' + '\nThis means your output selector will likely run more frequently than intended.' + '\nAvoid returning a new reference inside your input selector, e.g.' + - '\n`createSelector([(arg1, arg2) => ({ arg1, arg2 })],(arg1, arg2) => {})`' + '\n`createSelector([(arg1, arg2) => ({ arg1, arg2 })],(arg1, arg2) => {})`', + { + arguments, + firstInputs: params, + secondInputs: paramsCopy + } ) } diff --git a/test/inputStabilityCheck.spec.ts b/test/inputStabilityCheck.spec.ts index 5c3e005c4..be17bc427 100644 --- a/test/inputStabilityCheck.spec.ts +++ b/test/inputStabilityCheck.spec.ts @@ -32,12 +32,22 @@ describe('inputStabilityCheck', () => { expect(unstableInput).toHaveBeenCalledTimes(2) expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining('An input selector returned a different result') + expect.stringContaining('An input selector returned a different result'), + expect.objectContaining({ + // IArguments isn't an array :( + arguments: expect.anything(), + firstInputs: expect.arrayContaining([ + expect.objectContaining({ a: 1, b: 2 }) + ]), + secondInputs: expect.arrayContaining([ + expect.objectContaining({ a: 1, b: 2 }) + ]) + }) ) }) it('disables check if global setting is changed', () => { - setInputStabilityCheckEnabled(false) + setInputStabilityCheckEnabled('never') expect(addNums(1, 2)).toBe(3) @@ -45,12 +55,12 @@ describe('inputStabilityCheck', () => { expect(consoleSpy).not.toHaveBeenCalled() - setInputStabilityCheckEnabled(true) + setInputStabilityCheckEnabled('once') }) it('disables check if specified in the selector options', () => { const addNums = createSelector([unstableInput], ({ a, b }) => a + b, { - inputStabilityCheck: false + inputStabilityCheck: 'never' }) expect(addNums(1, 2)).toBe(3)