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

@@ -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)',
+      },
+    ],
   },
 }

src/lib/preact-redux.tsx

@@ -38,13 +38,24 @@ export function useDispatch(): Dispatch {
   return store.dispatch
 }
 
-export function useActionCreator<T, U>(creator: (payload: T) => Action<U>): (t: T) => void {
+export function useActionCreator<T, U>(
+  creator_: (payload: T) => Action<U>,
+  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<T, U>(selector: (t: T) => U): U {
+export function useSelector<T, U>(selector_: (t: T) => U, cacheArgs: any[]): U {
   const store = useStore<T>()
+
+  /* eslint-disable react-hooks/exhaustive-deps */
+  const selector = useCallback(selector_, cacheArgs)
+
   const getValueFromStore = useCallback(() => selector(store.getState()), [store, selector])
   const [value, setValue] = useState(getValueFromStore)
 

src/store/index.ts

@@ -71,6 +71,7 @@ export function createAppStore(initialState?: ApplicationState): redux.Store<App
   return redux.createStore(reducer, initialState)
 }
 
-export function useAppSelector<T>(selector: (t: ApplicationState) => T): T {
-  return useSelector(selector)
+export function useAppSelector<T>(selector: (t: ApplicationState) => T, cacheArgs: any[]): T {
+  /* eslint-disable react-hooks/exhaustive-deps */
+  return useSelector(selector, cacheArgs)
 }

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 (
     <Application
       activeProfileState={activeProfileState}
       canvasContext={canvasContext}
-      setGLCanvas={useActionCreator(actions.setGLCanvas)}
-      setLoading={useActionCreator(actions.setLoading)}
-      setError={useActionCreator(actions.setError)}
-      setProfileGroup={useActionCreator(actions.setProfileGroup)}
-      setDragActive={useActionCreator(actions.setDragActive)}
-      setViewMode={useActionCreator(actions.setViewMode)}
-      setFlattenRecursion={useActionCreator(actions.setFlattenRecursion)}
-      setProfileIndexToView={useActionCreator(actions.setProfileIndexToView)}
+      setGLCanvas={useActionCreator(setGLCanvas, [])}
+      setLoading={useActionCreator(setLoading, [])}
+      setError={useActionCreator(setError, [])}
+      setProfileGroup={useActionCreator(setProfileGroup, [])}
+      setDragActive={useActionCreator(setDragActive, [])}
+      setViewMode={useActionCreator(setViewMode, [])}
+      setFlattenRecursion={useActionCreator(setFlattenRecursion, [])}
+      setProfileIndexToView={useActionCreator(setProfileIndexToView, [])}
       {...appState}
     />
   )

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')

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<T> {
-  profileIndex: number
-  args: {
-    id: FlamechartID
-  } & T
-}
+const {
+  setHoveredNode,
+  setLogicalSpaceViewportSize,
+  setConfigSpaceViewportRect,
+  setSelectedNode,
+} = actions.flamechart
 
 export function useFlamechartSetters(id: FlamechartID, profileIndex: number): FlamechartSetters {
-  function useActionCreatorWithIndex<T, U>(
-    actionCreator: ActionCreator<WithFlamechartContext<U>>,
-    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],
     ),
   }
 }

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')

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 (
     <ProfileTableView