From dead3f9ad9f93620ee4beac8cf4cfeb990dfb0e5 Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Mon, 25 May 2020 19:10:40 -0700 Subject: [PATCH] Fix bug with bad caching of action creators (#281) Profile switching was subtly broken because action creators weren't being correctly re-bound due to a missing dependency in a `useCallback` call. I also tried to reduce boilerplate in this PR by adding additional exhaustive deps protection via eslint for `useSelector`, `useAppSelector`, and `useActionCreator`. The removes the need for using `useCallback` or each of those. Fixes #280 --- .eslintrc.js | 7 +- src/lib/preact-redux.tsx | 15 ++++- src/store/index.ts | 5 +- src/views/application-container.tsx | 65 +++++++++++-------- src/views/callee-flamegraph-view.tsx | 5 +- src/views/flamechart-view-container.tsx | 63 +++++++----------- src/views/inverted-caller-flamegraph-view.tsx | 5 +- src/views/profile-table-view.tsx | 10 +-- 8 files changed, 90 insertions(+), 85 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 411b336..ae8f562 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -10,6 +10,11 @@ module.exports = { rules: { '@typescript-eslint/explicit-function-return-type': 'off', 'react-hooks/rules-of-hooks': 'error', - 'react-hooks/exhaustive-deps': 'error', + 'react-hooks/exhaustive-deps': [ + 'error', + { + additionalHooks: '(useSelector|useAppSelector|useActionCreator)', + }, + ], }, } diff --git a/src/lib/preact-redux.tsx b/src/lib/preact-redux.tsx index 0941235..c9ff77c 100644 --- a/src/lib/preact-redux.tsx +++ b/src/lib/preact-redux.tsx @@ -38,13 +38,24 @@ export function useDispatch(): Dispatch { return store.dispatch } -export function useActionCreator(creator: (payload: T) => Action): (t: T) => void { +export function useActionCreator( + creator_: (payload: T) => Action, + cacheArgs: any[], +): (t: T) => void { const dispatch = useDispatch() + + /* eslint-disable react-hooks/exhaustive-deps */ + const creator = useCallback(creator_, cacheArgs) + return useCallback((t: T) => dispatch(creator(t)), [dispatch, creator]) } -export function useSelector(selector: (t: T) => U): U { +export function useSelector(selector_: (t: T) => U, cacheArgs: any[]): U { const store = useStore() + + /* eslint-disable react-hooks/exhaustive-deps */ + const selector = useCallback(selector_, cacheArgs) + const getValueFromStore = useCallback(() => selector(store.getState()), [store, selector]) const [value, setValue] = useState(getValueFromStore) diff --git a/src/store/index.ts b/src/store/index.ts index 8528855..da7f95e 100644 --- a/src/store/index.ts +++ b/src/store/index.ts @@ -71,6 +71,7 @@ export function createAppStore(initialState?: ApplicationState): redux.Store(selector: (t: ApplicationState) => T): T { - return useSelector(selector) +export function useAppSelector(selector: (t: ApplicationState) => T, cacheArgs: any[]): T { + /* eslint-disable react-hooks/exhaustive-deps */ + return useSelector(selector, cacheArgs) } diff --git a/src/views/application-container.tsx b/src/views/application-container.tsx index 45dbee6..20fc5c1 100644 --- a/src/views/application-container.tsx +++ b/src/views/application-container.tsx @@ -4,46 +4,55 @@ import {getProfileToView, getCanvasContext} from '../store/getters' import {actions} from '../store/actions' import {useActionCreator} from '../lib/preact-redux' import {memo} from 'preact/compat' -import {useCallback} from 'preact/hooks' import {useAppSelector} from '../store' +const { + setLoading, + setError, + setProfileGroup, + setDragActive, + setViewMode, + setGLCanvas, + setFlattenRecursion, + setProfileIndexToView, +} = actions + export const ApplicationContainer = memo(() => { - const appState = useAppSelector(useCallback(state => state, [])) + const appState = useAppSelector(state => state, []) const canvasContext = useAppSelector( - useCallback(state => (state.glCanvas ? getCanvasContext(state.glCanvas) : null), []), + state => (state.glCanvas ? getCanvasContext(state.glCanvas) : null), + [], ) - const activeProfileState: ActiveProfileState | null = useAppSelector( - useCallback(state => { - const {profileGroup} = state - if (!profileGroup) return null - if (profileGroup.indexToView >= profileGroup.profiles.length) return null + const activeProfileState: ActiveProfileState | null = useAppSelector(state => { + const {profileGroup} = state + if (!profileGroup) return null + if (profileGroup.indexToView >= profileGroup.profiles.length) return null - const index = profileGroup.indexToView - const profileState = profileGroup.profiles[index] - return { - ...profileGroup.profiles[profileGroup.indexToView], - profile: getProfileToView({ - profile: profileState.profile, - flattenRecursion: state.flattenRecursion, - }), - index: profileGroup.indexToView, - } - }, []), - ) + const index = profileGroup.indexToView + const profileState = profileGroup.profiles[index] + return { + ...profileGroup.profiles[profileGroup.indexToView], + profile: getProfileToView({ + profile: profileState.profile, + flattenRecursion: state.flattenRecursion, + }), + index: profileGroup.indexToView, + } + }, []) return ( ) diff --git a/src/views/callee-flamegraph-view.tsx b/src/views/callee-flamegraph-view.tsx index c640f2b..7eebc52 100644 --- a/src/views/callee-flamegraph-view.tsx +++ b/src/views/callee-flamegraph-view.tsx @@ -17,7 +17,6 @@ import {FlamechartWrapper} from './flamechart-wrapper' import {useAppSelector} from '../store' import {h} from 'preact' import {memo} from 'preact/compat' -import {useCallback} from 'preact/hooks' const getCalleeProfile = memoizeByShallowEquality< { @@ -51,8 +50,8 @@ const getCalleeFlamegraphRenderer = createMemoizedFlamechartRenderer() export const CalleeFlamegraphView = memo((ownProps: FlamechartViewContainerProps) => { const {activeProfileState} = ownProps const {index, profile, sandwichViewState} = activeProfileState - const flattenRecursion = useAppSelector(useCallback(state => state.flattenRecursion, [])) - const glCanvas = useAppSelector(useCallback(state => state.glCanvas, [])) + const flattenRecursion = useAppSelector(state => state.flattenRecursion, []) + const glCanvas = useAppSelector(state => state.glCanvas, []) if (!profile) throw new Error('profile missing') if (!glCanvas) throw new Error('glCanvas missing') diff --git a/src/views/flamechart-view-container.tsx b/src/views/flamechart-view-container.tsx index f9d620c..55b5244 100644 --- a/src/views/flamechart-view-container.tsx +++ b/src/views/flamechart-view-container.tsx @@ -3,7 +3,6 @@ import {FlamechartID, FlamechartViewState} from '../store/flamechart-view-state' import {CanvasContext} from '../gl/canvas-context' import {Flamechart} from '../lib/flamechart' import {FlamechartRenderer, FlamechartRendererOptions} from '../gl/flamechart-renderer' -import {ActionCreator} from '../lib/typed-redux' import {useActionCreator} from '../lib/preact-redux' import {Frame, Profile, CallTreeNode} from '../lib/profile' import {memoizeByShallowEquality} from '../lib/utils' @@ -19,7 +18,6 @@ import {ActiveProfileState} from './application' import {Vec2, Rect} from '../lib/math' import {actions} from '../store/actions' import {memo} from 'preact/compat' -import {useCallback} from 'preact/hooks' interface FlamechartSetters { setLogicalSpaceViewportSize: (logicalSpaceViewportSize: Vec2) => void @@ -28,51 +26,34 @@ interface FlamechartSetters { setSelectedNode: (node: CallTreeNode | null) => void } -interface WithFlamechartContext { - profileIndex: number - args: { - id: FlamechartID - } & T -} +const { + setHoveredNode, + setLogicalSpaceViewportSize, + setConfigSpaceViewportRect, + setSelectedNode, +} = actions.flamechart export function useFlamechartSetters(id: FlamechartID, profileIndex: number): FlamechartSetters { - function useActionCreatorWithIndex( - actionCreator: ActionCreator>, - map: (t: T) => U, - ): (t: T) => void { - const callback = useCallback( - (t: T) => { - const args = Object.assign({}, map(t), {id}) - return actionCreator({profileIndex, args}) - }, - [actionCreator, map], - ) - return useActionCreator(callback) - } - - const { - setHoveredNode, - setLogicalSpaceViewportSize, - setConfigSpaceViewportRect, - setSelectedNode, - } = actions.flamechart - return { - setNodeHover: useActionCreatorWithIndex( - setHoveredNode, - useCallback(hover => ({hover}), []), + setNodeHover: useActionCreator( + (hover: {node: CallTreeNode; event: MouseEvent} | null) => + setHoveredNode({profileIndex, args: {id, hover}}), + [profileIndex, id], ), - setLogicalSpaceViewportSize: useActionCreatorWithIndex( - setLogicalSpaceViewportSize, - useCallback(logicalSpaceViewportSize => ({logicalSpaceViewportSize}), []), + setLogicalSpaceViewportSize: useActionCreator( + (logicalSpaceViewportSize: Vec2) => + setLogicalSpaceViewportSize({profileIndex, args: {id, logicalSpaceViewportSize}}), + [profileIndex, id], ), - setConfigSpaceViewportRect: useActionCreatorWithIndex( - setConfigSpaceViewportRect, - useCallback(configSpaceViewportRect => ({configSpaceViewportRect}), []), + setConfigSpaceViewportRect: useActionCreator( + (configSpaceViewportRect: Rect) => + setConfigSpaceViewportRect({profileIndex, args: {id, configSpaceViewportRect}}), + [profileIndex, id], ), - setSelectedNode: useActionCreatorWithIndex( - setSelectedNode, - useCallback(selectedNode => ({selectedNode}), []), + setSelectedNode: useActionCreator( + (selectedNode: CallTreeNode | null) => + setSelectedNode({profileIndex, args: {id, selectedNode}}), + [profileIndex, id], ), } } diff --git a/src/views/inverted-caller-flamegraph-view.tsx b/src/views/inverted-caller-flamegraph-view.tsx index a415bc3..2dcad89 100644 --- a/src/views/inverted-caller-flamegraph-view.tsx +++ b/src/views/inverted-caller-flamegraph-view.tsx @@ -18,7 +18,6 @@ import {useAppSelector} from '../store' import {FlamechartWrapper} from './flamechart-wrapper' import {h} from 'preact' import {memo} from 'preact/compat' -import {useCallback} from 'preact/hooks' const getInvertedCallerProfile = memoizeByShallowEquality( ({ @@ -57,8 +56,8 @@ const getInvertedCallerFlamegraphRenderer = createMemoizedFlamechartRenderer({in export const InvertedCallerFlamegraphView = memo((ownProps: FlamechartViewContainerProps) => { const {activeProfileState} = ownProps let {profile, sandwichViewState, index} = activeProfileState - const flattenRecursion = useAppSelector(useCallback(state => state.flattenRecursion, [])) - const glCanvas = useAppSelector(useCallback(state => state.glCanvas, [])) + const flattenRecursion = useAppSelector(state => state.flattenRecursion, []) + const glCanvas = useAppSelector(state => state.glCanvas, []) if (!profile) throw new Error('profile missing') if (!glCanvas) throw new Error('glCanvas missing') diff --git a/src/views/profile-table-view.tsx b/src/views/profile-table-view.tsx index 95b3a86..752c010 100644 --- a/src/views/profile-table-view.tsx +++ b/src/views/profile-table-view.tsx @@ -353,25 +353,25 @@ interface ProfileTableViewContainerProps { activeProfileState: ActiveProfileState } +const {setTableSortMethod} = actions.sandwichView + export const ProfileTableViewContainer = memo((ownProps: ProfileTableViewContainerProps) => { const {activeProfileState} = ownProps const {profile, sandwichViewState, index} = activeProfileState if (!profile) throw new Error('profile missing') - const tableSortMethod = useAppSelector(useCallback(state => state.tableSortMethod, [])) + const tableSortMethod = useAppSelector(state => state.tableSortMethod, []) const {callerCallee} = sandwichViewState const selectedFrame = callerCallee ? callerCallee.selectedFrame : null const frameToColorBucket = getFrameToColorBucket(profile) const getCSSColorForFrame = createGetCSSColorForFrame(frameToColorBucket) - const setSelectedFrameWithIndex = useCallback( + const setSelectedFrame = useActionCreator( (selectedFrame: Frame | null) => { return actions.sandwichView.setSelectedFrame({profileIndex: index, args: selectedFrame}) }, [index], ) - - const setSelectedFrame = useActionCreator(setSelectedFrameWithIndex) - const setSortMethod = useActionCreator(actions.sandwichView.setTableSortMethod) + const setSortMethod = useActionCreator(setTableSortMethod, []) return (