help decrease re-rendering from react-aria hooks?

[csantos-nydig]

🙋 Feature Request

I'd like to see if react-aria hook could be more friendly and avoid unnecessary re-renders. Specially in hooks that return event handlers. for example useHover / useFocus / useFocusRing

🤔 Expected Behavior

returned event handlers such as onFocus / onBlur and others to be new instances only when their dependency changes

😯 Current Behavior

as soon as any of the hooks mentioned above are re-called new event handlers are returned.

💁 Possible Solution

probably introducing useCallback should help. Something like:

Index: packages/@react-aria/interactions/src/useFocusWithin.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/packages/@react-aria/interactions/src/useFocusWithin.ts b/packages/@react-aria/interactions/src/useFocusWithin.ts
--- a/packages/@react-aria/interactions/src/useFocusWithin.ts   (revision d6b47064ce98bfff50903e6e3147c6f88147de15)
+++ b/packages/@react-aria/interactions/src/useFocusWithin.ts   (date 1634874625411)
@@ -15,7 +15,7 @@
 // NOTICE file in the root directory of this source tree.
 // See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions

-import {FocusEvent, HTMLAttributes, useRef} from 'react';
+import {FocusEvent, HTMLAttributes, useCallback, useRef} from 'react';

 interface FocusWithinProps {
   /** Whether the focus within events should be disabled. */
@@ -36,45 +36,45 @@
 /**
  * Handles focus events for the target and its descendants.
  */
-export function useFocusWithin(props: FocusWithinProps): FocusWithinResult {
+export function useFocusWithin({onBlurWithin, isDisabled, onFocusWithin, onFocusWithinChange}: FocusWithinProps): FocusWithinResult {
   let state = useRef({
     isFocusWithin: false
-  }).current;
-
-  if (props.isDisabled) {
-    return {focusWithinProps: {}};
-  }
+  });

-  let onFocus = (e: FocusEvent) => {
-    if (!state.isFocusWithin) {
-      if (props.onFocusWithin) {
-        props.onFocusWithin(e);
+  let onFocus = useCallback((e: FocusEvent) => {
+    if (!state.current.isFocusWithin) {
+      if (onFocusWithin) {
+        onFocusWithin(e);
       }

-      if (props.onFocusWithinChange) {
-        props.onFocusWithinChange(true);
+      if (onFocusWithinChange) {
+        onFocusWithinChange(true);
       }

-      state.isFocusWithin = true;
+      state.current.isFocusWithin = true;
     }
-  };
+  }, [onFocusWithin, onFocusWithinChange]);

-  let onBlur = (e: FocusEvent) => {
+  let onBlur = useCallback((e: FocusEvent) => {
     // We don't want to trigger onBlurWithin and then immediately onFocusWithin again
     // when moving focus inside the element. Only trigger if the currentTarget doesn't
     // include the relatedTarget (where focus is moving).
-    if (state.isFocusWithin && !e.currentTarget.contains(e.relatedTarget as HTMLElement)) {
-      if (props.onBlurWithin) {
-        props.onBlurWithin(e);
+    if (state.current.isFocusWithin && !e.currentTarget.contains(e.relatedTarget as HTMLElement)) {
+      if (onBlurWithin) {
+        onBlurWithin(e);
+      }
+
+      if (onFocusWithinChange) {
+        onFocusWithinChange(false);
       }

-      if (props.onFocusWithinChange) {
-        props.onFocusWithinChange(false);
-      }
+      state.current.isFocusWithin = false;
+    }
+  }, [onBlurWithin, onFocusWithinChange]);

-      state.isFocusWithin = false;
-    }
-  };
+  if (isDisabled) {
+    return {focusWithinProps: {}};
+  }

   return {
     focusWithinProps: {

Index: packages/@react-aria/focus/src/useFocusRing.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/packages/@react-aria/focus/src/useFocusRing.ts b/packages/@react-aria/focus/src/useFocusRing.ts
--- a/packages/@react-aria/focus/src/useFocusRing.ts    (revision d6b47064ce98bfff50903e6e3147c6f88147de15)
+++ b/packages/@react-aria/focus/src/useFocusRing.ts    (date 1634874799105)
@@ -1,4 +1,4 @@
-import {HTMLAttributes, useState} from 'react';
+import {HTMLAttributes, useCallback, useState} from 'react';
 import {isFocusVisible, useFocus, useFocusVisibleListener, useFocusWithin} from '@react-aria/interactions';
 import {useRef} from 'react';

@@ -43,20 +43,20 @@
   let state = useRef({
     isFocused: false,
     isFocusVisible: autoFocus || isFocusVisible()
-  }).current;
+  });
   let [isFocused, setFocused] = useState(false);
-  let [isFocusVisibleState, setFocusVisible] = useState(() => state.isFocused && state.isFocusVisible);
+  let [isFocusVisibleState, setFocusVisible] = useState(() => state.current.isFocused && state.current.isFocusVisible);

-  let updateState = () => setFocusVisible(state.isFocused && state.isFocusVisible);
+  let updateState = useCallback(() => setFocusVisible(state.current.isFocused && state.current.isFocusVisible), []);

-  let onFocusChange = isFocused => {
-    state.isFocused = isFocused;
+  let onFocusChange = useCallback(isFocused => {
+    state.current.isFocused = isFocused;
     setFocused(isFocused);
     updateState();
-  };
+  }, [updateState]);

   useFocusVisibleListener((isFocusVisible) => {
-    state.isFocusVisible = isFocusVisible;
+    state.current.isFocusVisible = isFocusVisible;
     updateState();
   }, [], {isTextInput});

@@ -72,7 +72,7 @@

   return {
     isFocused,
-    isFocusVisible: state.isFocused && isFocusVisibleState,
+    isFocusVisible: state.current.isFocused && isFocusVisibleState,
     focusProps: within ? focusWithinProps : focusProps
   };
 }

🔦 Context and 💻 Examples

const MyComp = ({ theProp }) => {
  const { isHovered, hoverProps } = useHover({});
  return (
    <>
      <span>{theProp} has nothing to do with hover below</span>
      <h1 {...hoverProps}>Hover me</h1>
      <span>H1 is hovered?: {isHovered}</span>
    </>
  );
};

hoverProps returns completely new event handlers when theProp changes

[snowystinger]

Hey! thanks for the issue! undoubtedly there are improvements we can make for many of the hooks in this respect. Would you be willing to open a PR with your proposed changes?

In the meantime, you can also mitigate this by pushing the hoverable into a component, now the h1 is re-rendered, but the others aren't because of the useHover

const MyComp = ({ theProp }) => {
  const { isHovered, setIsHovered } = useState(false);
  return (
    <>
      <span>{theProp} has nothing to do with hover below</span>
      <HoverableHeader onHoverChange={setIsHovered}>Hover me</HoverableHeader>
      <span>H1 is hovered?: {isHovered}</span>
    </>
  );
};

const HoverableHeader = (props) => {
  const { isHovered, hoverProps } = useHover(props);
  return <h1 {...props} />;
}

[csantos1113]

Here is a first pass on just a few hooks: https://github.com/adobe/react-spectrum/pull/3045 if you're ok with the changes we should apply those to other hooks!