From a861ca4d8ecd6c5d3e5b42abf752a2a16c97f3f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nikolas=20Schr=C3=B6ter?= <25958801+nwidynski@users.noreply.github.com> Date: Tue, 4 Feb 2025 16:52:28 +0100 Subject: [PATCH] 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 --- packages/@react-aria/utils/src/mergeProps.ts | 16 ++--- packages/@react-aria/utils/src/useId.ts | 11 ++-- ...mergeProps.test.js => mergeProps.test.jsx} | 66 +++++++++++++++---- 3 files changed, 66 insertions(+), 27 deletions(-) rename packages/@react-aria/utils/test/{mergeProps.test.js => mergeProps.test.jsx} (57%) diff --git a/packages/@react-aria/utils/src/mergeProps.ts b/packages/@react-aria/utils/src/mergeProps.ts index eb5a2610330..95116d08e59 100644 --- a/packages/@react-aria/utils/src/mergeProps.ts +++ b/packages/@react-aria/utils/src/mergeProps.ts @@ -34,10 +34,10 @@ type UnionToIntersection = (U extends any ? (k: U) => void : never) extends ( * @param args - Multiple sets of props to merge together. */ export function mergeProps(...args: T): UnionToIntersection> { - // 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,7 +53,7 @@ export function mergeProps(...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 ( @@ -61,12 +61,12 @@ export function mergeProps(...args: T): UnionToIntersectio 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; } } } diff --git a/packages/@react-aria/utils/src/useId.ts b/packages/@react-aria/utils/src/useId.ts index 9d7640f65f3..4b9e6f85e03 100644 --- a/packages/@react-aria/utils/src/useId.ts +++ 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; diff --git a/packages/@react-aria/utils/test/mergeProps.test.js b/packages/@react-aria/utils/test/mergeProps.test.jsx similarity index 57% rename from packages/@react-aria/utils/test/mergeProps.test.js rename to packages/@react-aria/utils/test/mergeProps.test.jsx index d7915b5a36f..cd64fd06de7 100644 --- a/packages/@react-aria/utils/test/mergeProps.test.js +++ b/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) =>
); + + const Component = () => { + let id1 = 'id1'; + let id2 = useId('id2'); + + mergeProps({ id: id1 }, { id: id2 }); + + return + }; + + render(); + + // 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
; + }; + + expect(() => render()).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); + }); });