Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Table isRowHeader error (Next.js 13.3 + react-aria-components) #4390

Closed
willfiore opened this issue Apr 14, 2023 · 2 comments · Fixed by #4518
Closed

Table isRowHeader error (Next.js 13.3 + react-aria-components) #4390

willfiore opened this issue Apr 14, 2023 · 2 comments · Fixed by #4518
Labels
bug Something isn't working RAC

Comments

@willfiore
Copy link

🐛 Bug Report

Rendering a Table (w/ TableHeader, Column children etc.) throws the following error:

Error: A table must have at least one Column with the isRowHeader prop set to true

The error is thrown even if the prop is set on a Column.

  • Strict mode is disabled from next.config.js
  • SSRProvider is used

I can only reproduce the issue in Next.js 13.3.0. Works fine in 13.2.4.

💻 Code Sample

Next.js 13.3.0 - broken:
https://codesandbox.io/p/sandbox/react-aria-next-js-13-3-0-broken-table-071o7w?file=%2Fapp%2Fpage.tsx

Next.js 13.2.4 (otherwise identical) - works:
https://codesandbox.io/p/sandbox/react-aria-next-js-13-2-4-working-table-t8keyk?file=%2Fapp%2Fpage.tsx

@devongovett
Copy link
Member

Looks like Next.js is using an experimental version of react, roughly corresponding to the react@next tag on npm. This is only the case with the experimental appDir flag turned on. A recent behavior change in a refactor of React broke the hack we were using to pass through objects to "host" elements.

// HACK: the `multiple` prop is special in that React will pass it through as a property rather
// than converting to a string and using setAttribute. This allows our custom fake DOM to receive
// the props as an object. Once React supports custom elements, we can switch to that instead.
// https://github.com/facebook/react/issues/11347
// https://github.com/facebook/react/blob/82c64e1a49239158c0daa7f0d603d2ad2ee667a9/packages/react-dom/src/shared/DOMProperty.js#L386
// @ts-ignore
return <item multiple={{...props, rendered: props.children}} />;

Now React will coerce the object to a boolean when setting the property: https://github.com/facebook/react/blob/b6006201b5fdfcc5720160f169b80ddb7b8d7467/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L426-L429

We'll have to come up with an alternative.

@devongovett
Copy link
Member

devongovett commented Apr 17, 2023

I would refactor Item to something like this (same for other collection components - maybe we should make a helper):

function Item(props) {
  let ref = useRef();
  useLayoutEffect(() => {
    ref.current.setProps(props);
  }, [props]);

  return <item ref={ref} />
}

And then replace the multiple setter in ElementNode with a setProps method doing the same thing. I think the parentNode check that throws "Cannot change the id of an item" will fail then because layout effects are run after the element is already connected to its parent. So we'll need another flag to check if the props are being set for the first time or not.

This must be done in a layout effect so that it is completed before the useLayoutEffect at the root of the collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working RAC
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants