fix: infinite re-render on reoccuring ids

[nwidynski]

Closes #7655

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

[snowystinger]

Thanks for diagnosing and fixing this, what a tricky little bug

[nwidynski]

@LFDanLu Can we get an additional pair of eyes on this? Afaik, should be good to go 👍

[reidbarber]

Thanks for this!

[snowystinger]

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

[snowystinger]

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

[snowystinger]

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.

[nwidynski]

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 👍

[snowystinger]

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. 🔥