From 88dce648b9c3e7eeffbf0902345c4e43b49a54b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josep=20M=20Sobrepere=20Profit=C3=B3s?= Date: Thu, 2 May 2019 21:34:23 +0200 Subject: [PATCH] Avoid unnecessary selector evaluations --- src/hooks/useSelector.js | 80 ++++++++++++++++++++++------------ test/hooks/useSelector.spec.js | 60 ++++++++++++++++++++++++- 2 files changed, 110 insertions(+), 30 deletions(-) diff --git a/src/hooks/useSelector.js b/src/hooks/useSelector.js index 8e40cfa10..db403327c 100644 --- a/src/hooks/useSelector.js +++ b/src/hooks/useSelector.js @@ -1,4 +1,4 @@ -import { useReducer, useRef, useEffect, useMemo, useLayoutEffect } from 'react' +import { useState, useRef, useEffect, useMemo, useLayoutEffect } from 'react' import invariant from 'invariant' import { useReduxContext } from './useReduxContext' import shallowEqual from '../utils/shallowEqual' @@ -42,52 +42,36 @@ export function useSelector(selector) { invariant(selector, `You must pass a selector to useSelectors`) const { store, subscription: contextSub } = useReduxContext() - const [, forceRender] = useReducer(s => s + 1, 0) + const [subscriptionResult, setSubscriptionResult] = useState([null]) const subscription = useMemo(() => new Subscription(store, contextSub), [ store, contextSub ]) + const latestSelector = useRef() + const latestResult = useRef() const latestSubscriptionCallbackError = useRef() - const latestSelector = useRef(selector) + const latestSubscriptionResult = useRef(subscriptionResult) - let selectedState = undefined - - try { - selectedState = selector(store.getState()) - } catch (err) { - let errorMessage = `An error occured while selecting the store state: ${ - err.message - }.` - - if (latestSubscriptionCallbackError.current) { - errorMessage += `\nThe error may be correlated with this previous error:\n${ - latestSubscriptionCallbackError.current.stack - }\n\nOriginal stack trace:` - } - - throw new Error(errorMessage) - } - - const latestSelectedState = useRef(selectedState) + let result = latestResult.current useIsomorphicLayoutEffect(() => { + latestResult.current = result latestSelector.current = selector - latestSelectedState.current = selectedState latestSubscriptionCallbackError.current = undefined + latestSubscriptionResult.current = subscriptionResult }) useIsomorphicLayoutEffect(() => { function checkForUpdates() { + let newSelectedState try { - const newSelectedState = latestSelector.current(store.getState()) + newSelectedState = latestSelector.current(store.getState()) - if (shallowEqual(newSelectedState, latestSelectedState.current)) { + if (shallowEqual(newSelectedState, latestResult.current)) { return } - - latestSelectedState.current = newSelectedState } catch (err) { // we ignore all errors here, since when the component // is re-rendered, the selectors are called again, and @@ -96,7 +80,9 @@ export function useSelector(selector) { latestSubscriptionCallbackError.current = err } - forceRender({}) + const newSubscriptionResult = new Array(1) + newSubscriptionResult[0] = newSelectedState + setSubscriptionResult(newSubscriptionResult) } subscription.onStateChange = checkForUpdates @@ -107,5 +93,41 @@ export function useSelector(selector) { return () => subscription.tryUnsubscribe() }, [store, subscription]) - return selectedState + try { + // If the selector has changed, then it has to be re-evaluated + if (selector !== latestSelector.current) { + return (result = selector(store.getState())) + } + + // If the subscriptionResult is different, that means that this + // update has been triggered from the subscription + if (subscriptionResult != latestSubscriptionResult.current) { + // Before we return the result that was calculated during + // the subscription we need to check whether an error + // ocurred during the computation. If that is the case, + // then we re-evaluate the selector with the latest state + if (latestSubscriptionCallbackError.current) { + return (result = selector(store.getState())) + } + + // At this point we know for sure that it is safe to return + // the result that was computed inside the subscription + return (result = subscriptionResult[0]) + } + + // We just prevented an unnecessary re-evaluation of the selector! + return result + } catch (err) { + let errorMessage = `An error occured while selecting the store state: ${ + err.message + }.` + + if (latestSubscriptionCallbackError.current) { + errorMessage += `\nThe error may be correlated with this previous error:\n${ + latestSubscriptionCallbackError.current.stack + }\n\nOriginal stack trace:` + } + + throw new Error(errorMessage) + } } diff --git a/test/hooks/useSelector.spec.js b/test/hooks/useSelector.spec.js index c37c5b6e6..79fcaa90f 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, { useCallback, useReducer } from 'react' import { createStore } from 'redux' import { renderHook, act } from 'react-hooks-testing-library' import * as rtl from 'react-testing-library' @@ -31,6 +31,31 @@ describe('React', () => { expect(result.current).toEqual(0) }) + it('always uses the latest state', () => { + store = createStore(c => c + 1, -1) + + const Comp = () => { + const selector = useCallback(c => c + 1, []) + const value = useSelector(selector) + renderedItems.push(value) + return
+ } + + rtl.render( + + + + ) + + expect(renderedItems).toEqual([1]) + + act(() => { + store.dispatch({ type: '' }) + }) + + expect(renderedItems).toEqual([1, 2]) + }) + it('selects the state and renders the component when the store updates', () => { const { result } = renderHook(() => useSelector(s => s.count), { wrapper: props => @@ -156,6 +181,39 @@ describe('React', () => { }) }) + it('uses the latest selector', () => { + 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
+ } + + rtl.render( + + + + ) + + expect(renderedItems).toEqual([0]) + + rtl.act(forceRender) + expect(renderedItems).toEqual([0, 1]) + + rtl.act(() => { + store.dispatch({ type: '' }) + }) + expect(renderedItems).toEqual([0, 1]) + + rtl.act(forceRender) + expect(renderedItems).toEqual([0, 1, 2]) + }) + describe('edge cases', () => { it('ignores transient errors in selector (e.g. due to stale props)', () => { const spy = jest.spyOn(console, 'error').mockImplementation(() => {})