Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: infinite re-render on reoccuring ids #7657

Merged
merged 9 commits into from
Feb 4, 2025
Merged

fix: infinite re-render on reoccuring ids #7657

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f68765e
fix: infinite re-render on reoccuring ids
nwidynski Jan 23, 2025
eb10c95
fix: useId effect cleanup
nwidynski Jan 23, 2025
9552781
fix: react 19 compatibility
nwidynski Jan 23, 2025
ae733b7
fix: typo
nwidynski Jan 23, 2025
b416bad
fix: jest anything matcher
nwidynski Jan 23, 2025
d87d4c0
fix: match optional anything
nwidynski Jan 23, 2025
7295ea0
fix: react 16 fc & assertion
nwidynski Jan 23, 2025
8abeb3e
chore: remove nvmrc upgrade
nwidynski Jan 24, 2025
e82fc9a
Merge branch 'main' into id-fix
snowystinger Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
18.*
22.13.0
16 changes: 8 additions & 8 deletions packages/@react-aria/utils/src/mergeProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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--) {
Copy link
Member

@snowystinger snowystinger Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on this, I ran a benchmark in jest with this file's set of changes. It is very dependent on the objects being merged and their order.

As it was: 9876 ms
With changes: 10754 ms


describe('mergeProps', function () {
  it('benchmark', function () {
    let startTime = performance.now();
    for (let i = 0; i < 1000000; i++) {
      mergeProps(...data);
    }
    let endTime = performance.now();
    console.log('mergeProps', endTime - startTime);
  });
});

// many objects to merge with complex values and overlapping keys
let data = [{
  a: 1,
  b: 2,
  c: 3,
  d: 4,
  e: 5,
  f: 6,
  g: 7,
  h: 8,
  i: 9,
  j: 10,
  id: 'foo',
}, {
  h: 80,
  i: 90,
  j: 100,
  onClick: () => {},
  onMouseEnter: () => {},
  UNSAFE_className: 'foo',
  UNSAFE_style: { color: 'red' },
  id: 'bar',
},
{
  a: 10,
  b: 20,
  c: 30,
  d: 40,
  e: 50,
  f: 60,
  g: 70,
  h: 80,
  i: 90,
  j: 100,
  id: 'baz',
},
{
  a: 10,
  b: 20,
  c: 30,
  d: 40,
  e: 50,
  f: 60,
  g: 70,
  h: 80,
  i: 90,
  j: 100,
  id: 'qux',
},
{
  onClick: () => {},
  UNSAFE_className: 'bar',
  UNSAFE_style: { color: 'yello' },
  a: 10,
  b: 20,
  c: 30,
  d: 40,
  e: 50,
  id: 'quux',
},
{
  onFoo: () => {},
  UNSAFE_className: 'baz',
  UNSAFE_style: { color: 'green', display: 'block', position: 'absolute', top: 0, left: 0 },
  a: 100,
},
{
  onBar: () => {},
  UNSAFE_className: 'qux',
  UNSAFE_style: { color: 'blue', display: 'none', position: 'relative', bottom: 0, right: 0 },
  b: 200,
},
{
  onBaz: () => {},
  UNSAFE_className: 'quux',
  UNSAFE_style: { color: 'purple', display: 'flex', position: 'fixed', top: 0, right: 0 },
  c: 300,
},
{
  onQux: () => {},
  UNSAFE_className: 'quuz',
  UNSAFE_style: { color: 'pink', display: 'grid', position: 'sticky', bottom: 0, left: 0 },
  d: 400,
},
{
  onQuux: () => {},
  UNSAFE_className: 'quuz',
  UNSAFE_style: { color: 'orange', display: 'inline', position: 'static', bottom: 0, right: 0 },
  e: 500,
}];

but if I change the order of the objects being merged by using data = data.reverse()

As it was: 12110 ms
With changes: 7623 ms

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd need to check for actual usage in an application or benchmark our actual components, maybe TableView or something for more accurate measurements

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, tried it in our RAC Table tests using

  it('should allow selection even when drag and drop is disabled', async () => {
    let startTime = performance.now();
    for (let i = 0; i < 1000; i++) {
      let {unmount} = render(
        <DraggableTableWithSelection />
      );
      unmount();
    }
    let endTime = performance.now();
    console.log('Duration:', endTime - startTime);
  });

As it was: 12063 ms
With changes: 12300 ms

So, it's likely slower in real life.

Copy link
Contributor Author

@nwidynski nwidynski Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I guess this largely depends on where large objects are expected in the array, since the initial spread is doing most of the heavy lifting here.

I would have naively assumed them to occur in reverse order, as the first array item is often times user props while the later are usually hook props. I guess this is made obsolete by all the hooks often times just overriding a small portion of another hooks props, thereby favoring the prior version.

Thanks for giving this proper testing 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'm still not entirely convinced by one component, even if it's a fairly complex one. Given that one ordering gave a 1sec delta and the reverse gave a 5sec delta in that test, I'd love to see if we can't benefit from that better delta. But until we can come up with a way to test across all components with likely prop objects being added... this will do.

Thanks again for all the work on it. 🔥

let props = args[i];
for (let key in props) {
let a = result[key];
Expand All @@ -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;
}
}
}
Expand Down
11 changes: 6 additions & 5 deletions packages/@react-aria/utils/src/useId.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
66 changes: 52 additions & 14 deletions ...@react-aria/utils/test/mergeProps.test.js → ...react-aria/utils/test/mergeProps.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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();
Expand All @@ -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);
});
Expand All @@ -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);
});
});