Table: Duplicate item id causes infinite collection and out of memory crash

[nathanforce]

Provide a general summary of the issue here

After a recent react-aria upgrade we started seeing our Node servers tip over due to running out of memory. We managed to track the cause to be how we were constructing rows in our RAC based Table component.

Specifically, two or more rows sharing a single id when set via props on the Row element is where we saw the problem.

🤔 Expected Behavior?

Maybe we should warn in this case? Or ignore subsequent items with conflicting keys?

😯 Current Behavior

Page/app crashes once the array grows too large.

💁 Possible Solution

No response

🔦 Context

No response

🖥️ Steps to Reproduce

https://codesandbox.io/p/sandbox/winter-moon-szxxyj

Version

react-aria-components@1.7.1

What browsers are you seeing the problem on?

Chrome

If other, please specify.

Also in Node during SSR

What operating system are you using?

macos

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[snowystinger]

We do specifically say https://react-spectrum.adobe.com/react-aria/collections.html#unique-ids

I think we've discussed throwing an error if we see this, or a warning, like React does for duplicate keys.

We definitely don't want to ignore them, as that would be akin to failing silently and then users may not realize that they are missing data.

[nathanforce]

Given that the result here is a total crash of the browser tab (or the server in SSR), I don't think a warning is sufficient unless you somehow break the loop.

Agree that ignoring items is suboptimal and unexpected, but so is the current behavior.

[staticshock]

This affects not just Tables, but collections in general. The most insidious part of this bug is that it does not point to the part of your code responsible for the error, it just points here once it exceeds max array size.

If possible, I'd rather see RAC crash sooner, because the slowdown leading up to the crash, as memory is depleted, makes the whole experience feel even worse.

[staticshock]

I'm struggling to add an equivalent [generic] check in userland for this basic example:

<MyCollection items={items}>
  {(item) => <MyCollectionItem id={item.someId}>{item.value}</MyCollectionItem>}
</MyCollection>

Is there some way to generically catch duplicate ids inside MyCollection or MyCollectionItem? From the point of view of a single item I don't have any context on what other items got rendered, and from the point of view of the collection I don't know how the ids are going to be derived.

[snowystinger]

can you explain more about this comment?

from the point of view of the collection I don't know how the ids are going to be derived

I would expect that you can do something like (psuedocode)

useMemo(() => {
  let keys = new Set();
  for (let item of items) {
    if (keys.has(item.id /* or whatever your unique identifier is */) {
      throw, or return false or whatever you want to do
    }
    keys.add(item.id);
  }
}, [items])

If someone would like to contribute a check for this in our code, I think it shouldn't be too hard, just check if the key already exists in the keyMap and throw if it does

react-spectrum/packages/@react-aria/collections/src/BaseCollection.ts

Line 195 in d312e0a

this.keyMap.set(node.key, node);

[staticshock]

What I mean is that inside <MyCollection> I don't know what the id field name is going to be. In my example above, the key was item.someId, not item.id. Basically, it gets kind of ugly if the field name of the id field is not hardcoded to item.id. Otherwise I'd agree with you.

[staticshock]

I tried your suggestion of throwing an exception in BaseCollection, but it breaks hundreds of tests. Hopefully all of those failures can be fixed by updating fixtures, but I'm a bit out of my depth on making sure none of the failures are legitimate.