search

nextui-org / nextui

🚀 Beautiful, fast and modern React UI library.
https://nextui.org
MIT License
22.14k stars 1.56k forks source link

[BUG] - When using Table, "type.getCollectionNode is not a function" error #729

Open Clovel opened 2 years ago

Clovel commented 2 years ago

Describe the bug

When using the Table components, I get an error in the console from inside NextUI's code :

image

I tried using the Table component two different ways : 

<Table.Header>
  {
    counterTableColumns.map(
      (pColumn: CounterTableColumn) => {
        return (
          <Table.Column
            key={pColumn.uid}
            hideHeader={pColumn.hideHeader}
            align={pColumn.align ?? 'start'}
          >
            {pColumn.name}
          </Table.Column>
        );
      },
    )
  }
</Table.Header>

In both cases, no ESLint or TypeScript errors are displayed in my IDE nor in my terminal.

These test were also made with the Table.Body section of the table.

I am using :

Your Example Website or App

No response

Steps to Reproduce the Bug or Issue

CountersList.tsx :

/* Type declarations ----------------------------------- */
interface CounterTableColumn {
  name: string;
  uid: string;
  hideHeader?: boolean;
  align?: 'center' | 'start' | 'end';
}

/* Internal variables & constants ---------------------- */
const counterTableColumns: CounterTableColumn[] = [
  { name: 'Nom', uid: 'name' },
  { name: 'Description', uid: 'role' },
  { name: 'Actions', uid: 'actions', hideHeader: true },
];

const userCounters: CounterLightFragment[] = [
  {
    __typename: 'Counter',
    id: '1',
    name: 'Test1',
    description: 'Une description',
    avatarHash: null,
    avatarContentType: null,
  },
];

/* CountersList component prop types ------------------- */
interface CountersListProps {}

/* CountersList component ------------------------------ */
const CountersList: React.FC<CountersListProps> = () => {
  const userID = useReactiveVar(userIDReactiveVar);

  // const {
  //   data: userData,
  //   loading: loadingUserData,
  //   error: errorUserData,
  // } = useGetUserQuery(
  //   {
  //     variables: {
  //       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
  //       id: userID!,
  //     },
  //     skip: userID === null,
  //   },
  // );

  // const userCounters: CounterLightFragment[] = userData?.getUser?.counters?.map(
  //   (pVal: UserCounterOwnershipFragment) => {
  //     return pVal.counter;
  //   },
  // ) ?? [];

  console.log(`[DEBUG] <CountersList> userCounters :`, userCounters);

  return (
    <Table bordered>
      <Table.Header>
        {
          counterTableColumns.map(
            (pColumn: CounterTableColumn) => {
              return (
                <Table.Column
                  key={pColumn.uid}
                  hideHeader={pColumn.hideHeader}
                  align={pColumn.align ?? 'start'}
                >
                  {pColumn.name}
                </Table.Column>
              );
            },
          )
        }
      </Table.Header>
      {/* <Table.Header columns={counterTableColumns}>
        {
          (pColumn: CounterTableColumn) => {
            return (
              <Table.Column
                key={pColumn.uid}
                hideHeader={pColumn.hideHeader}
                align={pColumn.align ?? 'start'}
              >
                {pColumn.name}
              </Table.Column>
            );
          }
        }
      </Table.Header> */}
      <Table.Body>
        {
          userCounters.map(
            (pCounter: CounterLightFragment) => {
              return (
                <CountersListItem
                  key={`${pCounter.id}-${pCounter.name}`}
                  counter={pCounter}
                />
              );
            },
          )
        }
      </Table.Body>
      {/* <Table.Body items={userCounters}>
        {
          (pCounter: CounterLightFragment) => {
            return (
              <CountersListItem
                key={`${pCounter.id}-${pCounter.name}`}
                counter={pCounter}
              />
            );
          }
        }
      </Table.Body> */}
      <Table.Pagination
        shadow
        noMargin
        align="center"
        rowsPerPage={10}
        onPageChange={(page) => console.log({ page })}
      />
    </Table>
  );
};

/* Export CountersList component ----------------------- */
export default CountersList;

CountersListItem.tsx :

/* CountersListItem component prop types --------------- */
interface CountersListItemProps {
  counter: CounterLightFragment;
}

/* CountersListItem component -------------------------- */
const CountersListItem: React.FC<CountersListItemProps> = ({ counter }) => {
  return (
    <Table.Row key={counter.id}>
      <Table.Cell>
        <User
          src={counter.avatarHash ?? undefined}
          name={counter.name}
        >
          {counter.description}
        </User>
      </Table.Cell>
      <Table.Cell>
        {JSON.stringify(counter)}
      </Table.Cell>
      <Table.Cell>
        Actions
      </Table.Cell>
    </Table.Row>
  );
};

/* Export CountersListItem component ------------------- */
export default CountersListItem;

Expected behavior

Render the table correctly

Screenshots or Videos

image

Operating System Version

Windows 10

Browser

Chrome

jrgarciadev commented 2 years ago

Hey @Clovel could you try again using react:17?, React 18 is still not fully supported by react-aria which is the library used by NextUI under the hood for creating tables, could you create a codesanbox/repository with the issue?

Clovel commented 2 years ago

Using React & React DOM 17 completely broke my application.

Most errors come from ReactNode types being incompatible with React.ReactNode types, etc. Most likely I have a bunch of incompatible dependencies now.

for example, I have MUI v5 in my project alongside NextUI. MUI v5.10 required a version of @types/react-transition-group that requires React 18.

Type 'React.ReactNode' is not assignable to type 'import("C:/Users/<myUser>/repository/<project>/<project>-frontend/node_modules/@types/react-transition-group/node_modules/@types/react/index").ReactNode'.

Also, it seems that React 18 is listed as a peer dependency for the react-aria project you linked : 

  "peerDependencies": {
    "react": "^16.8.0 || ^17.0.0-rc.1 || ^18.0.0",
    "react-dom": "^16.8.0 || ^17.0.0-rc.1 || ^18.0.0"
  },

I tried reproducing the error in a sandbox, but it's not the same error... Anyway : https://codesandbox.io/s/nextui-react-18-error-tes3wg

Clovel commented 2 years ago

EDIT : I had a rogue

reidbarber commented 2 years ago

As mentioned in https://github.com/adobe/react-spectrum/issues/3502#issuecomment-1242241141, this error is due to using CountersListItem as children of Table.Body, when it's expecting Table.Row as children.

Clovel commented 2 years ago

As mentioned in adobe/react-spectrum#3502 (comment), this error is due to using CountersListItem as children of Table.Body, when it's expecting Table.Row as children.

Even if the CountersListItem has Table.Row as it's main component ?

reidbarber commented 2 years ago

Even if the CountersListItem has Table.Row as it's main component ?

Correct. Refactoring around that should work

jrgarciadev commented 2 years ago

Hey @Clovel as @devongovett (react-aria lead maintainer) mentioned here, The Table.Body component only supports Table.Row elements as children

TylerNRobertson commented 1 year ago

Any reason for this limitation? I don't see a reason why we couldn't just pass the TableRow a custom as prop or something like that. Main reason for this is you cannot integrate NextUI tables with any other library that has container elements. For example: react-beautiful-dnd basically works out of the box with every UI library I've tried before aside from NextUI

Also curious how the builder works, if it looks for a tr element within the table body and then find the cells in that, what does allowing container elements do that's so bad? I think all that would need to happen in that case is changing how the builder selects the elements, instead of looking for direct child elements, select all the tr elements only for example

defguru commented 1 year ago

This behavior is non sense.

We can not do something like this, which would make sense if you need to do some processing per row, or event if you just want to encapsulate the row logic...

 const MyRow = ({item, ...tableRowProps} : {item: FooItemType}) => {
    const status = useFetchItemStatus(item)
    return (
      <TableRow {...tableRowProps} key={`example-${item.id}`}>
          <TableCell>{item.foo}</TableCell>
          <TableCell>{status || "loading satus"}</TableCell>
      </TableRow>
    )
 }

.... 
// Then in the parent component you would do
...

<Table aria-label="Example table">
    <TableHeader>
        <TableColumn>Col A</TableColumn>
        <TableColumn>Col B</TableColumn>
    </TableHeader>
    <TableBody>
        {data.map(item => (
            <MyRow item={item} />
        ))}
    </TableBody>
</Table>

So honestly I don't understand the logic of not allowing this, this issue should remain opened imo since no solution was provided.

devongovett commented 1 year ago

It's not a limitation we wanted, just a technical trade off required by internal implementation details. React actually makes it extremely difficult to build components that are aware of their children like Table needs to be in order to implement things like keyboard navigation.

That said, in React Aria Components, our new component library built on top of React Aria hooks, we found a new way of implementing collection components that lifts this restriction. The goal is to extract this new implementation out once it is stable and offer it to React Aria hooks users such as Next UI as well. So hopefully in the near future Next UI will be able to lift the restriction as well.

In the meantime, we've recommended moving the logic to a component inside the item rather than a wrapper. You could refactor your example like the following:

<TableBody>
  {data.map(item => (
    <TableRow key={`example-${item.id}`}>
      <TableCell>{item.foo}</TableCell>
      <TableCell><ItemStatus item={item} /></TableCell>
    </TableRow>
  ))}
</TableBody>

function ItemStatus({item}) {
  const status = useFetchItemStatus(item)
  return status || 'loading status';
}
dailytravel commented 1 year ago

How I can add TableBody to <ReactSortable list={(column?.tasks || []).map((task) => ({ ...task, chosen: true, }))} setList={(newTasks) => { setColumns((prevColumns) => prevColumns.map((prevColumn) => prevColumn.id === column.id ? { ...prevColumn, tasks: newTasks } : prevColumn ) ); }} group="tasks" animation={150} tag={TableBody}

spot-developers commented 11 months ago

Have the same issue when trying to use DnD to reorder tabs. I'm assuming this is the same reliance on React Aria Components causing the underlying issue. Look forward to the updates.

sohrab- commented 11 months ago

What makes this quite cumbersome is that say if I have AccordionItems that do not render due to some logic inside them, now all that logic needs to be pulled up into the parent component so the AccordionItems can be conditionally removed. It basically promotes leaky abstractions. 😞

I did try to just assign getCollectionNode to my custom components. It does stop complaining but it doesn't actually render the custom component properly.

rydonahue1 commented 10 months ago

So is there currently no way to breakout NextUI Table components into separate components? If so that is pretty disappointing considering the benefits you can gain from splitting things into server and client components.

lazharichir commented 9 months ago

Same issue with the <Listbox> component... Considering the entire point of React is to breakdown complex UI into smaller components, this is a bit of a weird and unexpected behaviour.

divyam234 commented 9 months ago

@lazharichir MenuItem also

christo9090 commented 8 months ago

Yeah tried using the table again and got this same error. Unfortunately, it makes this table component fairly useless. Especially if you need to manage state for individual rows. There are refactoring work arounds but most of them are anti-patterns for react. Love this library otherwise.

Jaimayal commented 8 months ago

Ran into this same issue, had to workaround using the nuqs library to re-render the table on query parameters update. Hope we can get a proper way to implement logic/state per row soon!

viinaji commented 8 months ago

also got this issue with: DropdownItem

ameytessact commented 8 months ago

@devongovett @jrgarciadev How do you suggest using something like <InfiniteScroll /> from react-infinite-scroller with an array of AccordionItems?

alvaroaxo commented 7 months ago

@viinaji I have the same problem with DropdownItem, were you able to solve it?

viinaji commented 7 months ago

@alvaroaxo My goal was to separate each item in the menu, However, I encountered that error when using the component. To address this, I turned to the collections feature https://react-spectrum.adobe.com/react-stately/collections.html and the following is how I implemented it:

<DropdownMenu aria-label="Card Menu" items={CardMenuSections()}>
  {section => (
    <DropdownSection {...section}>
      {/* @ts-ignore:disable-next-line */}
      {item => <DropdownItem {...item} />}
    </DropdownSection>
  )}
</DropdownMenu>
type DropDownSectionType = {
  key: string;
  title?: string;
  showDivider?: boolean;
  items: (DropdownItemProps | undefined)[];
};
const CardMenuSections = (): DropDownSectionType[] => {
  return [
    {
      key: 'options',
      title: 'Options',
      showDivider: true,
      items: [LaunchConfig(), Extensions(), Pin()],
    },
    {
      key: 'update',
      title: 'Update',
      showDivider: true,
      items: [Update(), CheckForUpdate(), AutoUpdate()],
    },
    {
      key: 'about',
      title: 'About',
      items: [DocPage(), Info()],
    },
    {
      key: 'danger-zone',
      items: [Uninstall()],
    },
  ];
};
export const Pin = (): DropdownItemProps => {
  const {cardId} = useCardData();

  const isPinned = useIsPinnedCard(cardId);

  const onPress = () => window.api.storageMgr.pinnedCards(isPinned ? 'remove' : 'add', cardId);

  return {
    key: 'pin-unpin',
    title: isPinned ? 'Unpin' : 'Pin',
    startContent: GetIconByName('Pin', {
      className: `${isPinned ? 'rotate-45' : 'rotate-0'} transition duration-500`,
    }),
    className: 'cursor-default',
    onPress,
  };
};
neelhabib commented 5 months ago

I am facing same problem. Still no fix?

emmanuel-salvo commented 5 months ago

Why is this discussion marked as closed?

codemonkeylabs-de commented 5 months ago

Why is this discussion marked as closed?

Indeed... I just ran into this issue myself. Pretty disappointing...

Reading the history of the bug, it seems at some point the answer was "some smartie pants created some super smart scheme in some library and didn't think this thru so now this is a limitation, tough luck.. here is a workaround and go on with your lives"

Workaround is that instead of doing something like (in my case it happened with an Accordion component, but same thing applies to Tables etc)

<Accordion>
  <MyPrettyComponent>
    { item => <AccordionItem ..} ... >...</AccordionItem> } 
  </MyPrettyComponent>
</Accordion>

You refactor this to:

<MyNowNotSoPrettyComponent>{
  items =>
    <Accordion>{
        items.map(item => 
          <AccordionItem ...} ..> .. </AccordionItem>)
   }</Accordion>
 </MyNowNotSoPrettyComponent>

Worked for me.. and if you can't refactor your component for whatever reason, you are f.. ried.

niraj-khatiwada commented 5 months ago

I've found this issue to persist in almost every NextUI components that utilizes list items like Dropdown, ListBox, Select, Tabs, etc. To make a consistent theme in my App, I need to export the base component and all of its child component like this(image). This way I can just change any props from here and it'll be applied globally to my project. But this issue has created scattered abstraction everywhere I've to use the child component directly from nextui package instead of my @components. Really hoping for some help if someone finds any solution. image

saevarb commented 3 months ago

@devongovett

It's not a limitation we wanted, just a technical trade off required by internal implementation details. React actually makes it extremely difficult to build components that are aware of their children like Table needs to be in order to implement things like keyboard navigation.

That said, in React Aria Components, our new component library built on top of React Aria hooks, we found a new way of implementing collection components that lifts this restriction. The goal is to extract this new implementation out once it is stable and offer it to React Aria hooks users such as Next UI as well. So hopefully in the near future Next UI will be able to lift the restriction as well.

In the meantime, we've recommended moving the logic to a component inside the item rather than a wrapper. You could refactor your example like the following:

<TableBody>
  {data.map(item => (
    <TableRow key={`example-${item.id}`}>
      <TableCell>{item.foo}</TableCell>
      <TableCell><ItemStatus item={item} /></TableCell>
    </TableRow>
  ))}
</TableBody>

function ItemStatus({item}) {
  const status = useFetchItemStatus(item)
  return status || 'loading status';
}

Are there any updates on this?

Honestly, this argument really rings hollow. Everyone in the react ecosystem knows how to handle this. You are partially there; you just need to use inversion of control.

Instead of making it impossible to use standard and good practice for building one's react components, you just document the requirements of any child component(i.e. it must be forwardref, it must take this set of props and spread them on the top level element, etc) and use render props.

I've run into this issue trying to use almost every piece of this library and it's really making me reconsider using it, which is too bad. It was a candidate for becoming our primary internal component library at work and in my own personal projects. Unfortunately, the way it works now means that any react developer in our company that tries to use this library in a very sensible and reasonable way will run into extremely opaque errors. I would basically need to onboard everyone onto this issue specifically.

devongovett commented 3 months ago

Are there any updates on this?

Yes! In our last React Aria release, we introduced a new collection implementation that supports custom wrappers, available in the @react-aria/collections package. Here's an example of using it with React Aria hooks: https://stackblitz.com/edit/react-aria-collections?file=src%2FApp.tsx,package.json&terminal=dev

Next UI will need to do some work to adopt this in their implementation. One wrinkle is that there is a small breaking API change: you'll need to pass an id prop instead of/in addition to a key since we can no longer access React's key in the new implementation (React does not expose it inside component props). This means Next UI will either need to release a major version, or provide a way to opt-into the new API. I'm happy to work with the team to help with that if needed. Otherwise, after the internal changes in Next UI are made, it should be pretty transparent to users.

alexgradinar commented 3 months ago

@niraj-khatiwada were you able to find a solution?

niraj-khatiwada commented 3 months ago

@niraj-khatiwada were you able to find a solution?

Nope, no solution.

fwcd commented 1 month ago

Ran into this while trying to use react-intersection-observer to build a lazily loaded table by wrapping <TableRow>s in <InView>. Would be really useful to have the ability to wrap rows!

domhhv commented 1 month ago

I would also love to see a solution to this issue and an ability to move TableRow/TableCell to its own component, so that the various fetch requests of each table item is encapsulated.

HasanHaghniya commented 1 month ago

any updates?

pppdns commented 1 week ago

Next UI will need to do some work to adopt this in their implementation. [...] This means Next UI will either need to release a major version, or provide a way to opt-into the new API.

Any updates?