fix: infinite re-render on reoccuring ids (#7657)

* fix: infinite re-render on reoccuring ids

* fix: useId effect cleanup

* fix: react 19 compatibility

* fix: typo

* fix: jest anything matcher

* fix: match optional anything

* fix: react 16 fc & assertion

* chore: remove nvmrc upgrade

---------

Co-authored-by: Robert Snow <[email protected]>

packages/@react-aria/utils/src/mergeProps.ts

@@ -34,10 +34,10 @@ type UnionToIntersection<U> = (U extends any ? (k: U) => void : never) extends (
  * @param args - Multiple sets of props to merge together.
  */
 export function mergeProps<T extends PropsArg[]>(...args: T): UnionToIntersection<TupleTypes<T>> {
-  // Start with a base clone of the first argument. This is a lot faster than starting
+  // Start with a base clone of the last argument. This is a lot faster than starting
   // with an empty object and adding properties as we go.
-  let result: Props = {...args[0]};
-  for (let i = 1; i < args.length; i++) {
+  let result: Props = {...args[args.length - 1]};
+  for (let i = args.length - 2; i >= 0; i--) {
     let props = args[i];
     for (let key in props) {
       let a = result[key];
@@ -53,20 +53,20 @@ export function mergeProps<T extends PropsArg[]>(...args: T): UnionToIntersectio
         key.charCodeAt(2) >= /* 'A' */ 65 &&
         key.charCodeAt(2) <= /* 'Z' */ 90
       ) {
-        result[key] = chain(a, b);
+        result[key] = chain(b, a);
 
         // Merge classnames, sometimes classNames are empty string which eval to false, so we just need to do a type check
       } else if (
         (key === 'className' || key === 'UNSAFE_className') &&
         typeof a === 'string' &&
         typeof b === 'string'
       ) {
-        result[key] = clsx(a, b);
+        result[key] = clsx(b, a);
       } else if (key === 'id' && a && b) {
-        result.id = mergeIds(a, b);
+        result.id = mergeIds(b, a);
         // Override others
-      } else {
-        result[key] = b !== undefined ? b : a;
+      } else if (a === undefined) {
+        result[key] = b;
       }
     }
   }

packages/@react-aria/utils/src/useId.ts

@@ -54,14 +54,15 @@ export function useId(defaultId?: string): string {
     };
   }, [res]);
 
-  // This cannot cause an infinite loop because the ref is updated first.
+  // This cannot cause an infinite loop because the ref is always cleaned up.
   // eslint-disable-next-line
   useEffect(() => {
     let newId = nextId.current;
-    if (newId) {
-      nextId.current = null;
-      setValue(newId);
-    }
+    if (newId) { setValue(newId); }
+
+    return () => {
+      if (newId) { nextId.current = null; }
+    };
   });
 
   return res;

packages/@react-aria/utils/test/mergeProps.test.js → packages/@react-aria/utils/test/mergeProps.test.jsx

@@ -11,16 +11,16 @@
  */
 
 import clsx from 'clsx';
-import {mergeIds} from '../src/useId';
-import {mergeProps} from '../';
-
+import { mergeIds, useId } from '../src/useId';
+import { mergeProps } from '../src/mergeProps';
+import { render } from '@react-spectrum/test-utils-internal';
 
 describe('mergeProps', function () {
   it('handles one argument', function () {
-    let onClick = () => {};
+    let onClick = () => { };
     let className = 'primary';
     let id = 'test_id';
-    let mergedProps = mergeProps({onClick, className, id});
+    let mergedProps = mergeProps({ onClick, className, id });
     expect(mergedProps.onClick).toBe(onClick);
     expect(mergedProps.className).toBe(className);
     expect(mergedProps.id).toBe(id);
@@ -32,9 +32,9 @@ describe('mergeProps', function () {
     let message2 = 'click2';
     let message3 = 'click3';
     let mergedProps = mergeProps(
-      {onClick: () => mockFn(message1)},
-      {onClick: () => mockFn(message2)},
-      {onClick: () => mockFn(message3)}
+      { onClick: () => mockFn(message1) },
+      { onClick: () => mockFn(message2) },
+      { onClick: () => mockFn(message3) }
     );
     mergedProps.onClick();
     expect(mockFn).toHaveBeenNthCalledWith(1, message1);
@@ -51,14 +51,15 @@ describe('mergeProps', function () {
     let focus = 'focus';
     let margin = 2;
     const mergedProps = mergeProps(
-      {onClick: () => mockFn(click1)},
-      {onHover: () => mockFn(hover), styles: {margin}},
-      {onClick: () => mockFn(click2), onFocus: () => mockFn(focus)}
+      { onClick: () => mockFn(click1) },
+      { onHover: () => mockFn(hover), styles: { margin } },
+      { onClick: () => mockFn(click2), onFocus: () => mockFn(focus) }
     );
-
     mergedProps.onClick();
+    let callOrder = mockFn.mock.invocationCallOrder;
     expect(mockFn).toHaveBeenNthCalledWith(1, click1);
     expect(mockFn).toHaveBeenNthCalledWith(2, click2);
+    expect(callOrder[0]).toBeLessThan(callOrder[1]);
     mergedProps.onFocus();
     expect(mockFn).toHaveBeenNthCalledWith(3, focus);
     mergedProps.onHover();
@@ -71,7 +72,7 @@ describe('mergeProps', function () {
     let className1 = 'primary';
     let className2 = 'hover';
     let className3 = 'focus';
-    let mergedProps = mergeProps({className: className1}, {className: className2}, {className: className3});
+    let mergedProps = mergeProps({ className: className1 }, { className: className2 }, { className: className3 });
     let mergedClassNames = clsx(className1, className2, className3);
     expect(mergedProps.className).toBe(mergedClassNames);
   });
@@ -80,8 +81,45 @@ describe('mergeProps', function () {
     let id1 = 'id1';
     let id2 = 'id2';
     let id3 = 'id3';
-    let mergedProps = mergeProps({id: id1}, {id: id2}, {id: id3});
+    let mergedProps = mergeProps({ id: id1 }, { id: id2 }, { id: id3 });
     let mergedIds = mergeIds(mergeIds(id1, id2), id3);
     expect(mergedProps.id).toBe(mergedIds);
   });
+
+  it('combines ids with aria ids', function () {
+    let Spy = jest.fn((props) => <div {...props} />);
+
+    const Component = () => {
+      let id1 = 'id1';
+      let id2 = useId('id2');
+
+      mergeProps({ id: id1 }, { id: id2 });
+
+      return <Spy id={id2} />
+    };
+
+    render(<Component />);
+
+    // We use stringMatching to support optional refs in React 19.
+    expect(Spy).toHaveBeenCalledWith({ id: 'id2' }, expect.not.stringMatching(/\A(?!x)x/));
+    expect(Spy).toHaveBeenLastCalledWith({ id: 'id1' }, expect.not.stringMatching(/\A(?!x)x/));
+  });
+
+  it('combines reoccuring ids', function () {
+    const Component = () => {
+      let id1 = useId('id1');
+      let id2 = useId('id2');
+
+      return <div {...mergeProps({ id: id1 }, { id: id2 }, { id: id1 })} />;
+    };
+
+    expect(() => render(<Component />)).not.toThrow();
+  });
+
+  it('overrides other props', function () {
+    let id1 = 'id1';
+    let id2 = 'id2';
+    let mergedProps = mergeProps({ data: id1 }, { data: id2 });
+    expect(mergedProps.data).toBe(id2);
+  });
 });