[Button][base] Prevent too many ref updates (#33882)

mui · Aug 29, 2022 · a0c5385 · a0c5385
1 parent 5a9b953
commit a0c5385
Show file tree

Hide file tree

Showing 5 changed files with 190 additions and 30 deletions.
diff --git a/packages/mui-base/src/ButtonUnstyled/useButton.ts b/packages/mui-base/src/ButtonUnstyled/useButton.ts
@@ -1,6 +1,5 @@
 import * as React from 'react';
 import {
-  unstable_setRef as setRef,
   unstable_useForkRef as useForkRef,
   unstable_useIsFocusVisible as useIsFocusVisible,
 } from '@mui/utils';
@@ -9,7 +8,15 @@ import extractEventHandlers from '../utils/extractEventHandlers';
 import { EventHandlers } from '../utils/types';
 
 export default function useButton(parameters: UseButtonParameters) {
-  const { disabled = false, focusableWhenDisabled, href, ref, tabIndex, to, type } = parameters;
+  const {
+    disabled = false,
+    focusableWhenDisabled,
+    href,
+    ref: externalRef,
+    tabIndex,
+    to,
+    type,
+  } = parameters;
   const buttonRef = React.useRef<HTMLButtonElement | HTMLAnchorElement | HTMLElement>();
 
   const [active, setActive] = React.useState<boolean>(false);
@@ -148,13 +155,14 @@ export default function useButton(parameters: UseButtonParameters) {
     }
   };
 
-  const handleOwnRef = useForkRef(focusVisibleRef, buttonRef);
-  const handleRef = useForkRef(ref, handleOwnRef);
-
-  const updateRef = (instance: HTMLElement | null) => {
+  const updateHostElementName = React.useCallback((instance: HTMLElement | null) => {
     setHostElementName(instance?.tagName ?? '');
-    setRef(handleRef, instance);
-  };
+  }, []);
+
+  const handleRef = useForkRef(
+    updateHostElementName,
+    useForkRef(externalRef, useForkRef(focusVisibleRef, buttonRef)),
+  );
 
   interface AdditionalButtonProps {
     type?: React.ButtonHTMLAttributes<HTMLButtonElement>['type'];
@@ -209,7 +217,7 @@ export default function useButton(parameters: UseButtonParameters) {
       onMouseDown: createHandleMouseDown(externalEventHandlers),
       onMouseLeave: createHandleMouseLeave(externalEventHandlers),
       onMouseUp: createHandleMouseUp(externalEventHandlers),
-      ref: updateRef as React.Ref<any>,
+      ref: handleRef,
     };
   };
 

diff --git a/packages/mui-base/src/ListboxUnstyled/useControllableReducer.test.tsx b/packages/mui-base/src/ListboxUnstyled/useControllableReducer.test.tsx
@@ -0,0 +1,147 @@
+import { expect } from 'chai';
+import * as React from 'react';
+import { spy } from 'sinon';
+import { createRenderer } from 'test/utils';
+import useControllableReducer from './useControllableReducer';
+import {
+  ActionTypes,
+  ListboxAction,
+  ListboxState,
+  UseListboxPropsWithDefaults,
+} from './useListbox.types';
+
+describe('useControllableReducer', () => {
+  const { render } = createRenderer();
+
+  describe('dispatch', () => {
+    it('calls the provided internal reducer', () => {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      const reducer = spy((state: ListboxState<string>, action: ListboxAction<string>) => {
+        return state;
+      });
+
+      const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
+      const TestComponent = () => {
+        const props: UseListboxPropsWithDefaults<string> = {
+          options: ['a', 'b', 'c'],
+          defaultValue: 'a',
+          isOptionDisabled: () => false,
+          disableListWrap: false,
+          disabledItemsFocusable: false,
+          optionComparer: (a, b) => a === b,
+          optionStringifier: (option) => option,
+          multiple: false,
+        };
+        const [, dispatch] = useControllableReducer(reducer, undefined, props);
+        React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
+        return null;
+      };
+
+      render(<TestComponent />);
+      expect(reducer.getCalls()[0].args[1]).to.equal(actionToDispatch);
+    });
+
+    it('calls the provided external reducer', () => {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      const internalReducer = spy((state: ListboxState<string>, action: ListboxAction<string>) => {
+        return state;
+      });
+
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      const externalReducer = spy((state: ListboxState<string>, action: ListboxAction<string>) => {
+        return state;
+      });
+
+      const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
+      const TestComponent = () => {
+        const props: UseListboxPropsWithDefaults<string> = {
+          options: ['a', 'b', 'c'],
+          defaultValue: 'a',
+          isOptionDisabled: () => false,
+          disableListWrap: false,
+          disabledItemsFocusable: false,
+          optionComparer: (a, b) => a === b,
+          optionStringifier: (option) => option,
+          multiple: false,
+        };
+        const [, dispatch] = useControllableReducer(internalReducer, externalReducer, props);
+        React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
+        return null;
+      };
+
+      render(<TestComponent />);
+      expect(internalReducer.notCalled).to.equal(true);
+      expect(externalReducer.getCalls()[0].args[1]).to.equal(actionToDispatch);
+    });
+
+    it('calls onChange when the reducer returns a modified selected value', () => {
+      const reducer = spy((state: ListboxState<string>) => {
+        return {
+          ...state,
+          selectedValue: 'b',
+        };
+      });
+
+      const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
+      const handleChange = spy();
+      const handleHighlightChange = spy();
+
+      const TestComponent = () => {
+        const props: UseListboxPropsWithDefaults<string> = {
+          options: ['a', 'b', 'c'],
+          defaultValue: 'a',
+          isOptionDisabled: () => false,
+          disableListWrap: false,
+          disabledItemsFocusable: false,
+          optionComparer: (a, b) => a === b,
+          optionStringifier: (option) => option,
+          multiple: false,
+          onChange: handleChange,
+          onHighlightChange: handleHighlightChange,
+        };
+        const [, dispatch] = useControllableReducer(reducer, undefined, props);
+        React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
+        return null;
+      };
+
+      render(<TestComponent />);
+      expect(handleChange.getCalls()[0].args[0]).to.equal('b');
+      expect(handleHighlightChange.notCalled).to.equal(true);
+    });
+
+    it('calls onHighlightChange when the reducer returns a modified highlighted value', () => {
+      const reducer = spy((state: ListboxState<string>) => {
+        return {
+          ...state,
+          highlightedValue: 'b',
+        };
+      });
+
+      const actionToDispatch = { type: ActionTypes.setHighlight as const, highlight: 'b' };
+      const handleChange = spy();
+      const handleHighlightChange = spy();
+
+      const TestComponent = () => {
+        const props: UseListboxPropsWithDefaults<string> = {
+          options: ['a', 'b', 'c'],
+          defaultValue: 'a',
+          isOptionDisabled: () => false,
+          disableListWrap: false,
+          disabledItemsFocusable: false,
+          optionComparer: (a, b) => a === b,
+          optionStringifier: (option) => option,
+          multiple: false,
+          onChange: handleChange,
+          onHighlightChange: handleHighlightChange,
+        };
+        const [, dispatch] = useControllableReducer(reducer, undefined, props);
+        React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
+        return null;
+      };
+
+      render(<TestComponent />);
+      expect(handleHighlightChange.getCalls()[0].args[0]).to.equal('b');
+      expect(handleChange.notCalled).to.equal(true);
+    });
+  });
+});
diff --git a/packages/mui-base/src/ListboxUnstyled/useControllableReducer.ts b/packages/mui-base/src/ListboxUnstyled/useControllableReducer.ts
@@ -45,12 +45,16 @@ function useStateChangeDetection<TOption>(
   nextState: ListboxState<TOption>,
   internalPreviousState: ListboxState<TOption>,
   propsRef: React.RefObject<UseListboxPropsWithDefaults<TOption>>,
+  hasDispatchedActionRef: React.MutableRefObject<boolean>,
 ) {
   React.useEffect(() => {
-    if (!propsRef.current) {
+    if (!propsRef.current || !hasDispatchedActionRef.current) {
+      // Detect changes only if an action has been dispatched.
       return;
     }
 
+    hasDispatchedActionRef.current = false;
+
     const previousState = getControlledState(internalPreviousState, propsRef.current);
     const { multiple, optionComparer } = propsRef.current;
 
@@ -71,7 +75,7 @@ function useStateChangeDetection<TOption>(
         onChange?.(nextSelectedValue);
       }
     }
-  }, [nextState.selectedValue, internalPreviousState, propsRef]);
+  }, [nextState.selectedValue, internalPreviousState, propsRef, hasDispatchedActionRef]);
 
   React.useEffect(() => {
     if (!propsRef.current) {
@@ -101,6 +105,8 @@ export default function useControllableReducer<TOption>(
   const propsRef = React.useRef(props);
   propsRef.current = props;
 
+  const hasDispatchedActionRef = React.useRef(false);
+
   const initialSelectedValue =
     (value === undefined ? defaultValue : value) ?? (props.multiple ? [] : null);
 
@@ -111,6 +117,8 @@ export default function useControllableReducer<TOption>(
 
   const combinedReducer = React.useCallback(
     (state: ListboxState<TOption>, action: ListboxAction<TOption>) => {
+      hasDispatchedActionRef.current = true;
+
       if (externalReducer) {
         return externalReducer(getControlledState(state, propsRef.current), action);
       }
@@ -127,6 +135,11 @@ export default function useControllableReducer<TOption>(
     previousState.current = nextState;
   }, [previousState, nextState]);
 
-  useStateChangeDetection<TOption>(nextState, previousState.current, propsRef);
+  useStateChangeDetection<TOption>(
+    nextState,
+    previousState.current,
+    propsRef,
+    hasDispatchedActionRef,
+  );
   return [getControlledState(nextState, propsRef.current), dispatch];
 }
diff --git a/packages/mui-base/src/MultiSelectUnstyled/MultiSelectUnstyled.tsx b/packages/mui-base/src/MultiSelectUnstyled/MultiSelectUnstyled.tsx
@@ -77,7 +77,7 @@ function useUtilityClasses(ownerState: MultiSelectUnstyledOwnerState<any>) {
  */
 const MultiSelectUnstyled = React.forwardRef(function MultiSelectUnstyled<TValue>(
   props: MultiSelectUnstyledProps<TValue>,
-  ref: React.ForwardedRef<any>,
+  forwardedRef: React.ForwardedRef<any>,
 ) {
   const {
     autoFocus,
@@ -122,15 +122,11 @@ const MultiSelectUnstyled = React.forwardRef(function MultiSelectUnstyled<TValue
   const ListboxRoot = components.Listbox ?? 'ul';
   const Popper = components.Popper ?? PopperUnstyled;
 
-  const handleButtonRefChange = (element: HTMLElement | null) => {
-    buttonRef.current = element;
+  const handleButtonRefChange = React.useCallback((element: HTMLElement | null) => {
+    setButtonDefined(element != null);
+  }, []);
 
-    if (element != null) {
-      setButtonDefined(true);
-    }
-  };
-
-  const handleButtonRef = useForkRef(ref, handleButtonRefChange);
+  const handleButtonRef = useForkRef(forwardedRef, useForkRef(buttonRef, handleButtonRefChange));
 
   React.useEffect(() => {
     if (autoFocus) {

diff --git a/packages/mui-base/src/SelectUnstyled/SelectUnstyled.tsx b/packages/mui-base/src/SelectUnstyled/SelectUnstyled.tsx
@@ -69,7 +69,7 @@ function useUtilityClasses(ownerState: SelectUnstyledOwnerState<any>) {
  */
 const SelectUnstyled = React.forwardRef(function SelectUnstyled<TValue>(
   props: SelectUnstyledProps<TValue>,
-  ref: React.ForwardedRef<any>,
+  forwardedRef: React.ForwardedRef<any>,
 ) {
   const {
     autoFocus,
@@ -115,15 +115,11 @@ const SelectUnstyled = React.forwardRef(function SelectUnstyled<TValue>(
   const ListboxRoot = components.Listbox ?? 'ul';
   const Popper = components.Popper ?? PopperUnstyled;
 
-  const handleButtonRefChange = (element: HTMLElement | null) => {
-    buttonRef.current = element;
+  const handleButtonRefChange = React.useCallback((element: HTMLElement | null) => {
+    setButtonDefined(element != null);
+  }, []);
 
-    if (element != null) {
-      setButtonDefined(true);
-    }
-  };
-
-  const handleButtonRef = useForkRef(ref, handleButtonRefChange);
+  const handleButtonRef = useForkRef(forwardedRef, useForkRef(buttonRef, handleButtonRefChange));
 
   React.useEffect(() => {
     if (autoFocus) {