diff --git a/README.md b/README.md index 14f4454de..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) @@ -309,6 +313,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? 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 66711e0f9..66067705c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -38,6 +38,14 @@ export { defaultMemoize, defaultEqualityCheck } export type { DefaultMemoizeOptions } +type StabilityCheck = 'always' | 'once' | 'never' + +let globalStabilityCheck: StabilityCheck = 'once' + +export function setInputStabilityCheckEnabled(enabled: StabilityCheck) { + globalStabilityCheck = enabled +} + function getDependencies(funcs: unknown[]) { const dependencies = Array.isArray(funcs[0]) ? funcs[0] : funcs @@ -76,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() @@ -98,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. @@ -120,6 +127,15 @@ export function createSelectorCreator< ...finalMemoizeOptions ) + // @ts-ignore + const makeAnObject: (...args: unknown[]) => object = memoize( + // @ts-ignore + () => ({}), + ...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 @@ -131,7 +147,7 @@ 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 { length } = dependencies for (let i = 0; i < length; i++) { // apply arguments instead of spreading and mutate a local list of params for performance. @@ -139,8 +155,46 @@ export function createSelectorCreator< params.push(dependencies[i].apply(null, arguments)) } + const finalStabilityCheck = inputStabilityCheck ?? globalStabilityCheck + + if ( + process.env.NODE_ENV !== 'production' && + (finalStabilityCheck === 'always' || + (finalStabilityCheck === 'once' && firstRun)) + ) { + const paramsCopy = [] + + 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 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.' + + '\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) => {})`', + { + arguments, + firstInputs: params, + secondInputs: paramsCopy + } + ) + } + + if (firstRun) firstRun = false + } + // apply arguments instead of spreading for performance. lastResult = memoizedResultFunc.apply(null, params) + return lastResult } as F) @@ -164,7 +218,8 @@ export function createSelectorCreator< } export interface CreateSelectorOptions { - memoizeOptions: MemoizeOptions[0] | MemoizeOptions + memoizeOptions?: MemoizeOptions[0] | MemoizeOptions + inputStabilityCheck?: StabilityCheck } /** 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/inputStabilityCheck.spec.ts b/test/inputStabilityCheck.spec.ts new file mode 100644 index 000000000..be17bc427 --- /dev/null +++ b/test/inputStabilityCheck.spec.ts @@ -0,0 +1,122 @@ +import { createSelector, setInputStabilityCheckEnabled } from 'reselect' +import { shallowEqual } from 'react-redux' + +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() + }) + + it('calls each input selector twice, and warns to console if unstable reference is returned', () => { + const stableAddNums = createSelector( + [(a: number) => a, (a: number, b: number) => b], + (a, b) => a + b + ) + + expect(stableAddNums(1, 2)).toBe(3) + + expect(consoleSpy).not.toHaveBeenCalled() + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(2) + + expect(consoleSpy).toHaveBeenCalledWith( + 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('never') + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(1) + + expect(consoleSpy).not.toHaveBeenCalled() + + setInputStabilityCheckEnabled('once') + }) + + it('disables check if specified in the selector options', () => { + const addNums = createSelector([unstableInput], ({ a, b }) => a + b, { + inputStabilityCheck: 'never' + }) + + 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' + + expect(addNums(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(1) + + expect(consoleSpy).not.toHaveBeenCalled() + + 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], + ({ a, b }) => a + b, + { + memoizeOptions: { + equalityCheck: shallowEqual + } + } + ) + + expect(addNumsShallow(1, 2)).toBe(3) + + expect(unstableInput).toHaveBeenCalledTimes(2) + + expect(consoleSpy).not.toHaveBeenCalled() + }) +}) 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, 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"