From 6197dccd345aa40057eeb226042b9997bb9abc8b Mon Sep 17 00:00:00 2001
From: Jonathan Ziller <jonathan.ziller@gmail.com>
Date: Mon, 22 Apr 2019 17:37:02 +0200
Subject: [PATCH] add `deps` to `useSelector` to allow controlling stability of
 selector (#1251)

* fix stale selector issue

* add `deps` to `useSelector` to allow controlling stability of selector
---
 src/hooks/useSelector.js       | 16 ++++++++++----
 test/hooks/useSelector.spec.js | 39 +++++++++++++++++++++++++++-------
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/src/hooks/useSelector.js b/src/hooks/useSelector.js
index 397a93c9e..70fb70934 100644
--- a/src/hooks/useSelector.js
+++ b/src/hooks/useSelector.js
@@ -18,8 +18,14 @@ const useIsomorphicLayoutEffect =
 /**
  * A hook to access the redux store's state. This hook takes a selector function
  * as an argument. The selector is called with the store state.
+ *
+ * This hook takes a dependencies array as an optional second argument,
+ * which when passed ensures referential stability of the selector (this is primarily
+ * useful if you provide a selector that memoizes values).
  * 
  * @param {Function} selector the selector function
+ * @param {any[]} deps (optional) dependencies array to control referential stability
+ * of the selector
  * 
  * @returns {any} the selected state
  *
@@ -35,7 +41,7 @@ export const CounterComponent = () => {
 }
 ```
  */
-export function useSelector(selector) {
+export function useSelector(selector, deps) {
   invariant(selector, `You must pass a selector to useSelectors`)
 
   const { store, subscription: contextSub } = useReduxContext()
@@ -46,13 +52,15 @@ export function useSelector(selector) {
     contextSub
   ])
 
+  const memoizedSelector = useMemo(() => selector, deps)
+
   const latestSubscriptionCallbackError = useRef()
-  const latestSelector = useRef(selector)
+  const latestSelector = useRef(memoizedSelector)
 
   let selectedState = undefined
 
   try {
-    selectedState = latestSelector.current(store.getState())
+    selectedState = memoizedSelector(store.getState())
   } catch (err) {
     let errorMessage = `An error occured while selecting the store state: ${
       err.message
@@ -70,7 +78,7 @@ export function useSelector(selector) {
   const latestSelectedState = useRef(selectedState)
 
   useIsomorphicLayoutEffect(() => {
-    latestSelector.current = selector
+    latestSelector.current = memoizedSelector
     latestSelectedState.current = selectedState
     latestSubscriptionCallbackError.current = undefined
   })
diff --git a/test/hooks/useSelector.spec.js b/test/hooks/useSelector.spec.js
index a2fa2725e..411829c1d 100644
--- a/test/hooks/useSelector.spec.js
+++ b/test/hooks/useSelector.spec.js
@@ -1,6 +1,6 @@
 /*eslint-disable react/prop-types*/
 
-import React from 'react'
+import React, { useReducer } from 'react'
 import { createStore } from 'redux'
 import * as rtl from 'react-testing-library'
 import { Provider as ProviderMock, useSelector } from '../../src/index.js'
@@ -165,15 +165,40 @@ describe('React', () => {
 
           expect(renderedItems.length).toBe(1)
         })
+
+        it('re-uses the selector if deps do not change', () => {
+          let selectorId = 0
+          let forceRender
+
+          const Comp = () => {
+            const [, f] = useReducer(c => c + 1, 0)
+            forceRender = f
+            const renderedSelectorId = selectorId++
+            const value = useSelector(() => renderedSelectorId, [])
+            renderedItems.push(value)
+            return <div />
+          }
+
+          rtl.render(
+            <ProviderMock store={store}>
+              <Comp />
+            </ProviderMock>
+          )
+
+          rtl.act(forceRender)
+
+          // this line verifies the susbcription callback uses the same memoized selector and therefore
+          // does not cause a re-render
+          store.dispatch({ type: '' })
+
+          expect(renderedItems).toEqual([0, 0])
+        })
       })
 
       describe('edge cases', () => {
         it('ignores transient errors in selector (e.g. due to stale props)', () => {
-          // TODO Not sure this test is really testing what we want.
-          // TODO The parent re-renders, which causes the child to re-run the selector anyway and throw the error.
-          // TODO Had to flip the assertion for now. Probably needs to be rethought.
-
           const spy = jest.spyOn(console, 'error').mockImplementation(() => {})
+
           const Parent = () => {
             const count = useSelector(s => s.count)
             return <Child parentCount={count} />
@@ -197,9 +222,7 @@ describe('React', () => {
             </ProviderMock>
           )
 
-          expect(() => store.dispatch({ type: '' })).toThrowError(
-            /while selecting the store state/
-          )
+          expect(() => store.dispatch({ type: '' })).not.toThrowError()
 
           spy.mockRestore()
         })