adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.99k stars 1.13k forks source link

Passing nested row into table causes getCollectionNode error #2746

Open pontageorge opened 2 years ago

pontageorge commented 2 years ago

🐛 Bug Report

I have a table where I want to render a number of <Skeleton /> components that would contain a Row with five Cells both imported from the react-stately package. The Skeleton animation component is dispalyed while the table data waits to be retrieved from the API. Obviously I would like to keep this in a separate file from the actual table file.

🤔 Expected Behavior

The rows should render normally while data from the API loads.

😯 Current Behavior

This error is returned.

Call Stack
 $f8429209754fda4b9142d514065f4$export$CollectionBuilder.getFullNode$
  app.2b21bd4ce9b6247290f9.bundle.js:31712:853
 l
  app.2b21bd4ce9b6247290f9.bundle.js:33546:5798
 Generator._invoke
  app.2b21bd4ce9b6247290f9.bundle.js:33546:5582
 Generator.next
  app.2b21bd4ce9b6247290f9.bundle.js:33546:6224
 Object.n
  app.2b21bd4ce9b6247290f9.bundle.js:20551:4865
 _callee13$
  app.2b21bd4ce9b6247290f9.bundle.js:31717:269
 l
  app.2b21bd4ce9b6247290f9.bundle.js:33546:5798
 Generator._invoke
  app.2b21bd4ce9b6247290f9.bundle.js:33546:5582
 Generator.next
  app.2b21bd4ce9b6247290f9.bundle.js:33546:6224
 Object.n
  app.2b21bd4ce9b6247290f9.bundle.js:20551:4865

💁 Possible Solution

🔦 Context

If I render the Skeleton Row directly into the table component without passing it from the Skeleton file it works just fine. What I can tell is that the map function from the table component doesn't detect the nested components and throws this error.

💻 Code Sample

Table component

<Table scrollRef={ref} >
                        <Table.Head>
                            <Table.Heading align="center" className="border-r border-gray-300">
                               Head 1
                            </Table.Heading>
                            <Table.Heading align="center" className="border-r border-gray-300">
                                Head 2
                            </Table.Heading>
                            <Table.Heading align="center" className="border-r border-gray-300">
                                Head 3
                            </Table.Heading>
                            <Table.Heading align="center" className="border-r border-gray-300">
                                Head 4
                            </Table.Heading>
                            <Table.Heading align="center">
                                Head 5
                            </Table.Heading>
                        </Table.Head>
                    <Table.Body>
                            networkStatus
                                && (new Array(50)
                                    .fill({})).map((item, index) => (
                                    /* eslint-disable-next-line react/no-array-index-key */
                                        <Skeleton key={index} />
                                ))
              </Table.Body>
</Table>

Skeleton component

import React from "react";

import {
    PageSkeleton, Table,
} from "components";

function Skeleton() {
    return (
        <Table.Row>
            {(new Array(5).fill({})).map((cell, i) => (
              <Table.Cell key={i} className="border-r border-gray-300">
                  <PageSkeleton.Text />
              </Table.Cell>
              ))}
        </Table.Row>
    );
}

Working verison

<Table scrollRef={ref} >
                        <Table.Head>
                            <Table.Heading align="center" className="border-r border-gray-300">
                               Head 1
                            </Table.Heading>
                            <Table.Heading align="center" className="border-r border-gray-300">
                                Head 2
                            </Table.Heading>
                            <Table.Heading align="center" className="border-r border-gray-300">
                                Head 3
                            </Table.Heading>
                            <Table.Heading align="center" className="border-r border-gray-300">
                                Head 4
                            </Table.Heading>
                            <Table.Heading align="center">
                                Head 5
                            </Table.Heading>
                        </Table.Head>
                    <Table.Body>
                            networkStatus
                                && (new Array(50)
                                    .fill({})).map((item, index) => (
                                    /* eslint-disable-next-line react/no-array-index-key */
                                        <Table.Row>
                                          {(new Array(5).fill({})).map((cell, i) => (
                                            <Table.Cell key={i} className="border-r border-gray-300">
                                                <PageSkeleton.Text />
                                            </Table.Cell>
                                          ))}
                                        </Table.Row>
                                 ))
                       </Table.Body>
</Table>
LFDanLu commented 2 years ago

Out of curiosity, can you try adjusting your Skeleton component so that it has getCollectionNode similar to https://github.com/adobe/react-spectrum/blob/0f76dca420accd26c2f0f1ca73bb9ee82dc67b11/packages/%40react-spectrum/dialog/src/DialogTrigger.tsx#L121-L133?

EDIT: perhaps something like this:

function Skeleton() {
  return null;
}
Skeleton.getCollectionNode = function* getCollectionNode(props) {
  yield {
    element: (
        <Table.Row>
            {(new Array(5).fill({})).map((cell, i) => (
              <Table.Cell key={i} className="border-r border-gray-300">
                  <PageSkeleton.Text />
              </Table.Cell>
              ))}
        </Table.Row>
    )
  }
}
jamesonhill commented 2 years ago

I am also running into this issue. I want to do something like:

import { Item } from '@react-stately/collections'

export const ComboboxItem = (props) => <Item {...props} />;

But the above causes an immediate type.getCollectionNode is not a function fatal error. Is the above not the appropriate way to create a wrapper around <Item>?

LFDanLu commented 2 years ago

Mind expanding on expanding what you wanna do with the wrapper above? Perhaps can put anything custom within the Item itself.

@pontageorge What is your Table.Row and Table.Cell by the way? Is it the Row/Cell from the stately package?

jamesonhill commented 2 years ago

Mind expanding on expanding what you wanna do with the wrapper above? Perhaps can put anything custom within the Item itself.

@pontageorge What is your Table.Row and Table.Cell by the way? Is it the Row/Cell from the stately package?

I would like to constrain the types you can pass for the title and children props to <Item>. Both props are typed as ReactNode, which overly broad for my use case.

EddyVinck commented 1 year ago

I ran into the same problem with useSelect,<Item /> and the getCollectionNode error when I was modifying this tailwind example.

Instead of including Item in my custom DropdownItem component, I ended up getting it to work like this:

import type { DropdownProps } from './types';
import { Item, Select } from './Select';
import { DropdownItem } from './DropdownItem';

export const Dropdown = ({ items }: DropdownProps) => {
    return (
        <Select
            label='Favorite Animal'
            defaultSelectedKey={items[0].id}
            onSelectionChange={(key) => {
                console.log({ key });
            }}
        >
            {items.map((item) => (
                <Item key={item.id} textValue={item.title}>
                    <DropdownItem item={item} />
                </Item>
            ))}
        </Select>
    );
};
LFDanLu commented 1 year ago

Just an update, but @devongovett is currently experimenting with changes to collections as part of https://github.com/adobe/react-spectrum/discussions/2368 that will allow for custom wrappers around things like Item.

Charlie85270 commented 1 year ago

Hey guys, do you have updates on that ?

LFDanLu commented 1 year ago

No updates as of yet unfortunately.

mryechkin commented 1 year ago

I ran into the same problem with useSelect,<Item /> and the getCollectionNode error when I was modifying this tailwind example.

Instead of including Item in my custom DropdownItem component, I ended up getting it to work like this: ...

@EddyVinck you're a life-saver, thank you! 🙌

I was getting this error only in my tests, and I was re-exporting Item as part of my custom Select, ie:

<Select items={items}>
  {(item) => (
    <Select.Item key={item.value} textValue={item.value}>
      {item.label}
    </Select.Item>
  )}
</Select>

Switching from Select.Item to Item imported directly from react-stately fixed the issue, as you suggested.

Any ideas why this is happening? There were no errors when actually using the component.

snowystinger commented 1 year ago

As mentioned above, collections don't currently support wrapping Item.

However, we've been experimenting with a way to allow this, you can try it out in our React Aria Components. See https://react-spectrum.adobe.com/react-aria/Select.html#reusable-wrappers Eventually, we'll have this in all the docs and components, but for now it's just React Aria Components.

mryechkin commented 1 year ago

Thanks for the info @snowystinger.

In my case it's not a wrapper, but simply a re-export of Item from react-stately for convenience:

import { Item } from 'react-stately';

import Select from './Select'; // my custom Select

export default Object.assign(Select, { Item });

This way users can just import the custom Select and use Select.Item instead of having to import Item separately.

Are you suggesting that this is a problematic approach? I'm only using React Aria hooks, haven't converted to Components yet.

snowystinger commented 1 year ago

Hmmm I'm not sure why that is causing a problem specifically. You could check if the function, getCollectionNode, defined on Item gets carried over with the assign. It should be, but maybe some other piece is wrong due to the assign.

It's not that it's a problematic approach, so much as a path we haven't tested. It's fine to use React Aria hooks, they don't reference the new collection work yet is all. That work is in the React Aria Components, and it's currently an alpha while we make sure it's in a stable state with an API we like. Eventually, the hooks docs will reference the new work.

marasco commented 8 months ago

How is this open after 2 years ? Same issue here

snowystinger commented 8 months ago

Wrapping the various collection components is supported. See https://react-spectrum.adobe.com/react-aria/Table.html#reusable-wrappers

We still need to update the documentation, but you can always reference the RAC implementation to see how to get the same API's.

yusijs commented 7 months ago

@snowystinger will this also make it to the hooks? I got stuck on this when building a Select-component based on the hooks & wrapping from react-stately.

snowystinger commented 7 months ago

The documentation? yes, eventually, that's what the tag means on this issue.

If you mean, can you use it in the hooks, the answer should be yes already. You can reference our RAC implementation to see how we do it. RAC is built on the hooks. You'll need to import Collection stuff from the 'react-aria-components' package. The only snafu I'm aware of is that I don't know how much we've exported yet. It should be all available though.

yusijs commented 7 months ago

The documentation? yes, eventually, that's what the tag means on this issue.

Sorry, didn't notice the documentation-label on the issue, my bad. :)

If you mean, can you use it in the hooks, the answer should be yes already. You can reference our RAC implementation to see how we do it. RAC is built on the hooks. You'll need to import Collection stuff from the 'react-aria-components' package. The only snafu I'm aware of is that I don't know how much we've exported yet. It should be all available though.

The error I'm having is trying to create a custom wrapper around the component from react-aria (same error as @EddyVinck had in https://github.com/adobe/react-spectrum/issues/2746#issuecomment-1323575490).

This happens from this code:

function _Option<T>({ value, children, ...props }: OptionProps<T>) {
  return (
    <Item {...props} key={`select-list-${value}`}>
      {children}
    </Item>
  );
}

Which is due to getCollectionNode missing. I'll try looking at the RAC components (I used those directly at first and it worked, but wanted to look at using the hooks instead, as we're using the hooks for some other stuff as well).

snowystinger commented 7 months ago

Most likely you're following the hooks documentation which pulls collections from react-stately. If you look at https://github.com/adobe/react-spectrum/blob/759ac41ae9683d053044c508d9ec0dccebd3b8dd/packages/react-aria-components/src/ListBox.tsx#L14 you'll see that RAC has a different, newer set of APIs for collections. They're fairly similar, but were breaking from the old set, so we only implemented them for RAC to start.

yusijs commented 6 months ago

They're fairly similar, but were breaking from the old set, so we only implemented them for RAC to start.

This is what my question is really about; will those changes make it to react-aria's hooks at some point? I meant to answer a few days back, but several (if not all) of those hooks were not exposed in RAC, so we are "forced" to use the components from RAC.

snowystinger commented 6 months ago

Forgive me if I'm missing a nuance in your questions, but it feels like we're going in circles.

You can already use the new collections. You aren't forced to use the components from RAC. You just have to use collections from there. We haven't updated the docs yet for the hooks. You can refer to the RAC components for how to use the new collections with the hooks.

I haven't tried it yet, so if we missed exporting something, that might be the hang up here?

yusijs commented 6 months ago

Sorry, I'm trying to find a good way to formulate my question but it's a bit painful 😅

What I want to achieve: Create a reusable select-component based on the useSelect-hook in react-aria that can be used more or less like this:

<MySelect>
  <MyOption value="123">A Label</MyOption>

This is possible to achieve with the components from RAC, but not with only hooks from what I can find. I did manage to get it working eventually by replicating the getCollectionNode function used on Item from react-stately, but this is obviously not a great solution.

So my questions:

  1. Is it possible to achieve this with just the hooks? a. And if not: will the hooks/collections in react-aria / react-stately be updated to allow for this?
  2. When you say "use the new collections" from RAC; the line you're linking to has multiple imports, none of which are exported publicly from the react-aria-components package - is there some other way I'm supposed to import them that I'm just not seeing? :)

TIA; hopefully I managed to formulate my questions a bit better this time 😅

(also sorry for the noise on this issue)

snowystinger commented 6 months ago

No worries, looks like I'm in the wrong. When I skimmed that list of exports, I thought I saw the new collections. You could use patch-package to add it to the exports if you wanted to use it. We'll add it eventually because we do want to move all our hooks docs to it, as well as our React Spectrum implementation at some point. It's much more flexible than the precursor. I don't know if we'll add it early though. I think the last hold-up on it is an API discussion now that I'm remembering more. More info here https://github.com/adobe/react-spectrum/pull/5912

snowystinger commented 2 months ago

Just updating that the custom renders have been released and we're looking for API feedback note, it's in alpha right now, so there aren't docs for these features yet. looking at how we're using it in our source is the best way to find examples right now

yusijs commented 2 months ago

Awesome, I'll give it a look sometime this week when I get the time!