useMergedRefs should return an immutable function (microsoft#13712)

* useMergedRefs update to be immutable. * adding comment. * Change files * Updated to use refs. * removing unused import. * Updating api. * updating comments
levithomason · Jun 19, 2020 · 2ad2572 · 2ad2572
1 parent b0b4d70
commit 2ad2572
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 64 deletions.
diff --git a/change/@uifabric-react-hooks-2020-06-19-12-26-25-fix-button-fixes.json b/change/@uifabric-react-hooks-2020-06-19-12-26-25-fix-button-fixes.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "useMergedRefs: Merging refs should produce an immutable ref object.",
+  "packageName": "@uifabric/react-hooks",
+  "email": "dzearing@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-06-19T19:26:25.030Z"
+}
diff --git a/packages/react-hooks/etc/react-hooks.api.md b/packages/react-hooks/etc/react-hooks.api.md
@@ -1,53 +1,54 @@
-## API Report File for "@uifabric/react-hooks"
-
-> Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/).
-
-```ts
-
-import { Async } from '@uifabric/utilities';
-import * as React from 'react';
-
-// @public (undocumented)
-export type ChangeCallback<TElement extends HTMLElement, TValue> = (ev: React.FormEvent<TElement> | undefined, newValue: TValue | undefined) => void;
-
-// @public
-export interface IUseBooleanCallbacks {
-    setFalse: () => void;
-    setTrue: () => void;
-    toggle: () => void;
-}
-
-// @public
-export function useAsync(): Async;
-
-// @public
-export function useBoolean(initialState: boolean): [boolean, IUseBooleanCallbacks];
-
-// @public
-export function useConst<T>(initialValue: T | (() => T)): T;
-
-// @public
-export function useConstCallback<T extends (...args: any[]) => any>(callback: T): T;
-
-// @public
-export function useControllableValue<TValue, TElement extends HTMLElement>(controlledValue: TValue | undefined, defaultUncontrolledValue: TValue | undefined): Readonly<[TValue | undefined, (newValue: TValue | undefined) => void]>;
-
-// @public (undocumented)
-export function useControllableValue<TValue, TElement extends HTMLElement, TCallback extends ChangeCallback<TElement, TValue> | undefined>(controlledValue: TValue | undefined, defaultUncontrolledValue: TValue | undefined, onChange: TCallback): Readonly<[TValue | undefined, (newValue: TValue | undefined, ev?: React.FormEvent<TElement>) => void]>;
-
-// @public
-export function useForceUpdate(): () => void;
-
-// @public
-export function useId(prefix?: string, providedId?: string): string;
-
-// @public
-export function useMergedRefs<T>(...refs: React.Ref<T>[]): (instance: T) => void;
-
-// @public
-export function useOnEvent<TElement extends Element, TEvent extends Event>(element: React.RefObject<TElement | undefined | null> | TElement | Window | undefined | null, eventName: string, callback: (ev: TEvent) => void, useCapture?: boolean): void;
-
-
-// (No @packageDocumentation comment for this package)
-
-```
+## API Report File for "@uifabric/react-hooks"
+
+> Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/).
+
+```ts
+
+import { Async } from '@uifabric/utilities';
+import * as React from 'react';
+import { Ref } from 'react';
+
+// @public (undocumented)
+export type ChangeCallback<TElement extends HTMLElement, TValue> = (ev: React.FormEvent<TElement> | undefined, newValue: TValue | undefined) => void;
+
+// @public
+export interface IUseBooleanCallbacks {
+    setFalse: () => void;
+    setTrue: () => void;
+    toggle: () => void;
+}
+
+// @public
+export function useAsync(): Async;
+
+// @public
+export function useBoolean(initialState: boolean): [boolean, IUseBooleanCallbacks];
+
+// @public
+export function useConst<T>(initialValue: T | (() => T)): T;
+
+// @public
+export function useConstCallback<T extends (...args: any[]) => any>(callback: T): T;
+
+// @public
+export function useControllableValue<TValue, TElement extends HTMLElement>(controlledValue: TValue | undefined, defaultUncontrolledValue: TValue | undefined): Readonly<[TValue | undefined, (newValue: TValue | undefined) => void]>;
+
+// @public (undocumented)
+export function useControllableValue<TValue, TElement extends HTMLElement, TCallback extends ChangeCallback<TElement, TValue> | undefined>(controlledValue: TValue | undefined, defaultUncontrolledValue: TValue | undefined, onChange: TCallback): Readonly<[TValue | undefined, (newValue: TValue | undefined, ev?: React.FormEvent<TElement>) => void]>;
+
+// @public
+export function useForceUpdate(): () => void;
+
+// @public
+export function useId(prefix?: string, providedId?: string): string;
+
+// @public
+export function useMergedRefs<T>(...refs: Ref<T>[]): (instance: T) => void;
+
+// @public
+export function useOnEvent<TElement extends Element, TEvent extends Event>(element: React.RefObject<TElement | undefined | null> | TElement | Window | undefined | null, eventName: string, callback: (ev: TEvent) => void, useCapture?: boolean): void;
+
+
+// (No @packageDocumentation comment for this package)
+
+```
diff --git a/packages/react-hooks/src/useMergedRefs.test.tsx b/packages/react-hooks/src/useMergedRefs.test.tsx
@@ -1,8 +1,31 @@
 import * as React from 'react';
-import { mount } from 'enzyme';
+import { mount, ReactWrapper } from 'enzyme';
 import { useMergedRefs } from './useMergedRefs';
 
 describe('useMergedRefs', () => {
+  let wrapper: ReactWrapper | undefined;
+
+  afterEach(() => {
+    wrapper?.unmount();
+    wrapper = undefined;
+  });
+
+  it('always returns the same ref (refs should be immutable)', () => {
+    let lastMergedRef;
+
+    const TestComponent: React.FunctionComponent = () => {
+      lastMergedRef = useMergedRefs<boolean>(() => ({}));
+      return null;
+    };
+
+    wrapper = mount(<TestComponent />);
+    const ref1 = lastMergedRef;
+    wrapper.setProps({});
+    const ref2 = lastMergedRef;
+
+    expect(ref1).toBe(ref2);
+  });
+
   it('updates all provided refs', () => {
     const refObject: React.RefObject<boolean> = React.createRef<boolean>();
     let refValue: boolean | null = null;
@@ -11,7 +34,7 @@ describe('useMergedRefs', () => {
       mergedRef(true);
       return null;
     };
-    mount(<TestComponent />);
+    wrapper = mount(<TestComponent />);
 
     expect(refObject.current).toBe(true);
     expect(refValue).toBe(true);
@@ -29,7 +52,7 @@ describe('useMergedRefs', () => {
       return null;
     };
 
-    const wrapper = mount(<TestComponent />);
+    wrapper = mount(<TestComponent />);
 
     const firstRefCallback = refCallback;
 
@@ -51,7 +74,7 @@ describe('useMergedRefs', () => {
       return null;
     };
 
-    const wrapper = mount(<TestComponent />);
+    wrapper = mount(<TestComponent />);
 
     let secondRefValue: boolean | null = null;
     refValueFunc = (val: boolean) => (secondRefValue = val);

diff --git a/packages/react-hooks/src/useMergedRefs.ts b/packages/react-hooks/src/useMergedRefs.ts
@@ -1,19 +1,24 @@
-import * as React from 'react';
+import { useRef, useCallback, Ref, MutableRefObject } from 'react';
 
 /**
  * React hook to merge multiple React refs (either MutableRefObjects or ref callbacks) into a single ref callback that
  * updates all provided refs
  * @param refs- Refs to collectively update with one ref value.
  */
-export function useMergedRefs<T>(...refs: React.Ref<T>[]): (instance: T) => void {
-  return React.useCallback((value: T) => {
-    refs.forEach(ref => {
+export function useMergedRefs<T>(...refs: Ref<T>[]): (instance: T) => void {
+  const state = useRef<(Ref<T> | undefined)[]>();
+
+  // Update refs list.
+  state.current = refs;
+
+  return useCallback((value: T) => {
+    for (const ref of state.current!) {
       if (typeof ref === 'function') {
         ref(value);
       } else if (ref) {
         // work around the immutability of the React.Ref type
-        ((ref as unknown) as React.MutableRefObject<T>).current = value;
+        ((ref as unknown) as MutableRefObject<T>).current = value;
       }
-    });
-  }, refs);
+    }
+  }, []);
 }