Merge pull request #612 from reduxjs/stability-check

Author: markerikson

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?

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",

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> = {
-      memoizeOptions: undefined
-    }
+    let directlyPassedOptions: CreateSelectorOptions<MemoizeOptions> = {}
 
     // 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,16 +147,54 @@ 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.
         // @ts-ignore
         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 extends unknown[]> {
-  memoizeOptions: MemoizeOptions[0] | MemoizeOptions
+  memoizeOptions?: MemoizeOptions[0] | MemoizeOptions
+  inputStabilityCheck?: StabilityCheck
 }
 
 /**

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', () => {