Setting Picker Item key prop causes keys to be coerced to a string regardless of the actual type

[bmingles]

Provide a general summary of the issue here

The Picker component supports items with numeric id / key props.

Item.key unset

If the Item.key prop is not explicitly set, keys get derived from the id or key props on the items array.

e.g.

function Example() {
  let items = [
    { id: 1, name: 'Aardvark' },
    { id: 2, name: 'Cat' },
    { id: 3, name: 'Dog' },  ];
    
  let [animalId, setAnimalId] = React.useState(null);

  return (
    <>
      <Picker
        label="Pick an animal"
        items={items}
        onSelectionChange={setAnimalId}
        value={animalId}
      >
        { /* Item.key not explicitly set */
          (item) => <Item>{item.name}</Item>
        }
      </Picker>
      <p>Animal id type: {typeof animalId}</p>
    </>
  );
}

The onSelectionChange handler will pass a numeric key to setAnimalId (consistent with the items data) and value + defaultValue will work with values of type number.

Item.key explicitly set

If the Item.key prop is explicitly set e.g.

<Item key={item.id}>{item.name}</Item>}

The onSelectionChange will pass a string id to setAnimalId, and the value + defaultValue props will require values of type string in order to work.

🤔 Expected Behavior?

I would expect in the case of setting Item.key prop explicitly:

• onSelectionChange should pass the actual value (not the stringified version) of the original id / key
• value, and defaultValue should be able to match the id / key prop on the items instead of requiring them to be string values

😯 Current Behavior

When Item.key prop is explicitly set

• onSelectionChange passes a stringified value of the original id / key
• value, and defaultValue don't match the items id / key prop unless they are stringified

💁 Possible Solution

No response

🔦 Context

We have a wrapper component that needs to explicitly set the Item.key prop based on properties on the items. In cases where these props are numeric or boolean types, this forces the input value + onSelectedValue change handlers to work with strings which doesn't match the original data.

🖥️ Steps to Reproduce

https://codesandbox.io/p/sandbox/modern-shadow-znqpwf

Version

3.34.1

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

MacOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[snowystinger]

if you change from key to id, it should work. I'm not entirely sure the reason why key doesn't, but I know we're trying not to use it in favor of id so maybe something was overlooked.

[sookmax]

I might be terribly wrong here, but I think it has to do with the fact that the key provided to <Item> is essentially the underlying React Component's key, and it's extracted as-is in CollectionBuilder:

https://github.com/adobe/react-spectrum/blob/1293adf00a11ecac84ec9ab19af7dcea970af2b2/packages/%40react-stately/collections/src/CollectionBuilder.ts#L127

https://github.com/adobe/react-spectrum/blob/1293adf00a11ecac84ec9ab19af7dcea970af2b2/packages/%40react-stately/collections/src/CollectionBuilder.ts#L64-L84

Above, element passed as the first argument of this.getkey() is the object representation of <Item key={item.id}> (in our context) containing the provided key, and we extract this React key verbatim in the first if statement of getKey().

Thing is, React always seems to convert the provided key to string. (I found this stack exchange answer). For example if you specify something like <Item key={{hello: 'world'}}>, you'll see the element object would be of form: {$$typeof: Symbol(react.element), key: '[object Object]', ref: null, props: {…}, type: ƒ, …}

[bmingles]

@snowystinger I don't think this has anything to do with id vs key but is a side effect of how the React key prop handles values as suggsted by @sookmax

It looks like this is actually documented in the React Stately docs which I missed before: https://react-spectrum.adobe.com/react-stately/collections.html#unique-keys

Note that if you do specify a custom key on each Item using the key prop, React will convert the provided key to a string. As a result, all the collection component's key related event handlers and props (e.g. onSelectionChange, selectedKey(s)) will expect and return strings. If you forgo specifying a key on the Item and instead rely on the id or key being defined in your items array, the key related event handlers and props will match what you have in your items array.

This is unfortunate since in my particular use case, I need to use the key prop since it may not always originate from the same prop on the item data.

Another thought that comes to mind would be if the Picker component supported an optional getKey prop that could be used to derive the key from an item. That could allow users to not have to set the Item.key explicitly and instead control how the component derives the key from the item.

[snowystinger]

Yes, @sookmax has the correct analysis. I'm not sure I follow why you must use key and can't change to using id though. Could you explain more?

We've discussed a getKey before, I've forgotten the resolution of that discussion. I'll bring it up with the team to see if anyone recalls.

[bmingles]

In my case we have implemented Windowed data where data on items is lazy loaded based on scroll position. The item data looks something like:

interface {
   key: Key,
   data?: { realKey: number }
}

Where the key prop is a stable incrementing key that is always present, but the data.realKey is not.

To render this we use:

(item) => <Item key={item.data?.realKey ?? item.key}>{...}</Item>

This lets us incorporate the real key once it exists but fallback to the stable key that is always present if data isn't loaded for the item. I'm not aware of any way to coalesce props using the implicit keys, so have to be explicit.

[snowystinger]

Would you still be able to use id?

(item) => <Item id={item.data?.realKey ?? item.key}>{...}</Item>

[bmingles]

Types don't show that <Item> supports an id prop.

[snowystinger]

That's interesting, I thought it was. It appears to works if you use it, so I'm not sure why it's not in the types. We can look into it.

[sookmax]

Maybe I'm doing something wrong, but it doesn't seem to work with id on <Item> regardless the missing type?

I tested locally with the following story I temporarily created in Picker.stories.tsx:

export const PickerTest: PickerStory = {
  args: {
    onSelectionChange: key => {
      console.log(key, typeof key);
    },
    items: [
      {fakeKey: 1, realKey: 11, name: 'Aardvark'},
      {fakeKey: 2, name: 'Kangaroo'},
      {fakeKey: 3, realKey: 33, name: 'Snake'},
      {fakeKey: 4, name: 'Danni'},
      {fakeKey: 5, realKey: 55, name: 'Devon'},
      {fakeKey: 6, name: 'Ross'},
      {fakeKey: 7, realKey: 77, name: 'Puppy'},
      {fakeKey: 8, name: 'Doggo'},
      {fakeKey: 9, realKey: 99, name: 'Floof'}
    ],
    children: (item: any) => {
      return <Item id={item.realKey ?? item.fakeKey}>{item.name}</Item>;
    }
  }
};

And it gives you the following runtime error when mounted:

The error is inside CollectionBuilder.getKey() and it's throwing because it only checks the item's (here in variable v) key or id properties. Note that here v is not the React component <Item> but rather the data item (here the first one in the list: {fakeKey: 1, realKey: 11, name: 'Aardvark'})

[bmingles]

@sookmax I assume that will be a bigger issue if key or id actually exist on the data since it could get a value that doesn't match the id set on Item. @snowystinger this may be more than a TypeScript types problem. I don't think the id prop quite solves this use case.

[sookmax]

@bmingles

In fact, id set on <Item> doesn't seem to be considered at all in CollectionBuilder.getKey(). Only the React key prop on <Item> (prioritized) and key (prioritized) or id on the data item are considered as candidates of key: https://github.com/adobe/react-spectrum/blob/1293adf00a11ecac84ec9ab19af7dcea970af2b2/packages/%40react-stately/collections/src/CollectionBuilder.ts#L64-L84

this may be more than a TypeScript types problem. I don't think the id prop quite solves this use case.

Yeah I agree.

an optional getKey prop that could be used to derive the key from an item. That could allow users to not have to set the Item.key explicitly and instead control how the component derives the key from the item.

This sounds like a good idea to me too.

[snowystinger]

Thanks for the link to that piece of code @sookmax, I had forgotten that it was only key on the item which was used and that the other options came from the data object.

I think you could make use of the key attribute on the data objects themselves though, or id. And just not use the key prop on Item?

[
      {fakeKey: 1, key: 11, name: 'Aardvark'},
      {fakeKey: 2, name: 'Kangaroo'},
      {fakeKey: 3, key: 33, name: 'Snake'},
      {fakeKey: 4, name: 'Danni'},
      {fakeKey: 5, key: 55, name: 'Devon'},
      {fakeKey: 6, name: 'Ross'},
      {fakeKey: 7, key: 77, name: 'Puppy'},
      {fakeKey: 8, name: 'Doggo'},
      {fakeKey: 9, key: 99, name: 'Floof'}
    ].map(item => {
      if (item.key === undefined) {
        return {...item, key: item.fakeKey};
      }
      return key;
    })

[bmingles]

@snowystinger that would probably work but for my use case that would require mapping over the entire list any time an item changes which seems not ideal since our lists could be 10K-100K items.

[sookmax]

@snowystinger

Yeah you're right in that the user can always reorganize (duplicate) their data to fit the constraints (here in our case, to have fields named id or key).

I just thought it would be nicer to give the user a way to specify the field to be used as key, without them having to preprocess their data to generate a possibly duplicate field for key.

But at the same time, I'm not sure how we could go about achieving that. Well roughly maybe would something like this work?

private getKey(item: CollectionElement<T>, partialNode: PartialNode<T>, state: CollectionBuilderState, parentKey?: Key): Key {
  if (item.key != null) {
    return item.key;
  }

  if (partialNode.type === 'cell' && partialNode.key != null) {
    return `${parentKey}${partialNode.key}`;
  }

  let v = partialNode.value as any;
  if (v != null) {
--  let key = v.key ?? v.id;
++  let key;
++  if (item.props.getKey) {
++    key = item.props.getKey(v);
++  } else {
++    key = v.key ?? v.id;
++  }
    if (key == null) {
      throw new Error('No key found for item');
    }

    return key;
  }

  return parentKey ? `${parentKey}.${partialNode.index}` : `$.${partialNode.index}`;
}

So that user can specify their key like:

(item) => <Item getKey={(item) => item.data?.realKey ?? item.key}>{...}</Item>

[snowystinger]

@snowystinger that would probably work but for my use case that would require mapping over the entire list any time an item changes which seems not ideal since our lists could be 10K-100K items.

Not ideal, but theoretically your items aren't changing that frequently. So if you're running into an issue, you could speed this up by doing your own caching so that you don't have {...item, key: item.fakeKey} for every item. Also, this happens in linear time.

@sookmax agreed this may be nice; I'm bringing it up with the team this week, hopefully.

Also of note for anyone coming across this, we do change this in the RAC version to use the id prop instead of the key prop. https://github.com/adobe/react-spectrum/blob/e6923d9473264b972974513dacbcfab5a24d5616/packages/react-aria-components/src/Collection.tsx#L677 apologies for the confusion.

[qmhc]

Is there any update?

[snowystinger]

No updates of note. It's low priority as it has a workaround. The fastest way to see this change would be to submit a PR for further discussion on implementation.

[bmingles]

@snowystinger The proposed workaround does not appear to work for the use case I am reporting. Data can change very frequently in which case the mapping is a problem.