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

Compound components cause "React.jsx: type is invalid -- expected a string or a class/function" #74585

Closed
sebastianvitterso opened this issue Jan 7, 2025 · 12 comments
Labels
bug Issue was opened via the bug report template.

Comments

@sebastianvitterso
Copy link

Link to the code that reproduces this issue

https://github.com/sebastianvitterso/next-compound-component-issues

To Reproduce

Two "ways to break":
First:

  1. Start the app using npm run dev.
  2. Uncomment "use client" in components/Compound/index.tsx.
  3. Broken

Second:

  1. Start the app using npm run dev.
  2. Uncomment EDS' <Table> and <Table.Body>-code in app/page.tsx.
  3. Broken

Video showing:

Screen.Recording.2025-01-07.at.13.48.40.mov

Current vs. Expected behavior

For some reason, compound components "randomly" break when used in conjunction with "use client"-components within server-side pages.
Expectation: No error, just works.

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  System Firmware Version: 11881.61.3
Binaries:
  Node: 22.11.0
  npm: 10.9.0
Relevant Packages:
  next: 15.1.1-canary.26
  react: 19.0.0
  react-dom: 19.0.0
  typescript: 5.7.2
  @equinor/eds-core-react: 0.43.0-dev-01062025
Next.js Config: Default

Which area(s) are affected? (Select all that apply)

Not sure

Which stage(s) are affected? (Select all that apply)

next dev (local)

Additional context

No response

@sebastianvitterso sebastianvitterso added the bug Issue was opened via the bug report template. label Jan 7, 2025
@sebastianvitterso sebastianvitterso changed the title Compound components cause Compound components cause React.jsx: type is invalid -- expected a string or a class/function Jan 7, 2025
@sebastianvitterso sebastianvitterso changed the title Compound components cause React.jsx: type is invalid -- expected a string or a class/function Compound components cause "React.jsx: type is invalid -- expected a string or a class/function" Jan 7, 2025
@pawelblaszczyk5
Copy link

If you mark a file with “use client” and import it from server components the content of the module is replaced with references. You can’t access values off from references - hence it breaks

@sebastianvitterso
Copy link
Author

Alright, then I guess this might be more suited as a discussion, but could you point me in a direction for how we need to adjust the Equinor EDS core library (current PR: equinor/design-system#3716) to remedy this? As it currently stands, it doesn't work with these compound/namespaced components. Where do we need to add "use client" directives, and where must we avoid them? What are the rules, per se?

@pawelblaszczyk5
Copy link

I think many of them shouldn’t be marked with “use client” at all because they have unserializable props and they aren’t suitable as boundaries. Check out this MR from React Aria - adobe/react-spectrum#5826

@sebastianvitterso
Copy link
Author

But most of these components are very renderable both serverside and clientside, it's just that some of them contain hooks and must therefore have the "use client" directive at the top of the file. Is this notion wrong?

What does it mean that they have "unserializable props"?

@pawelblaszczyk5
Copy link

But most of these components are very renderable both serverside and clientside, it's just that some of them contain hooks and must therefore have the "use client" directive at the top of the file. Is this notion wrong?

What does it mean that they have "unserializable props"?

“use client” isn’t meant for marking client components. It’s supposed to mark client components which are boundaries in the application - these which are used directly from server components. These have additional restrictions, e.g. they need to have serializable props. Serializable props are documented here. So if your component e.g. Popover accepts anchorEl (HTMLElement) or onClose (callback) - these naturally can’t be marked as boundaries.

@sebastianvitterso
Copy link
Author

Ok, so that's understandable - but does that mean that we can not use these components (components that take HTMLElement or callback as props) at all in our application? How should the library authors make it possible to use these components both in SPA's and (e.g.) Nextjs-applications?

Adding a dependency to "client only" seems strange, but is that the only way? And if all that does is raise an error during build time, surely that isn't really a solution either?

@pawelblaszczyk5
Copy link

I think that for generic UI libraries such as React Aria or yours (if my brief check is correct) not using “use client” and opting for import client-only is a proper call. These components often can’t be rendered from server component anyway. And you as an author of generic UI library have no idea where boundaries will be in consumer app. At the same time import client-only clearly indicates that some component is meant to be used from client environment and it protects your from adding/removing a hook being a breaking change.

@sebastianvitterso
Copy link
Author

If I understand correcly, however, the "client-only" package only "helps" by throwing an error during the build-step if rendering a "client component" is attempted on the server side of things. I'm not sure if this helps us. If importing the "client only" package makes it so that library users themselves have to wrap all package-components in their own "use client"-components, then that seems overly complicated, and something that needs an easier solution.

I guess the core of what I'm wondering (and the reason behind this github issue) is:
What changes we must do to our library to ensure it works well for SPAs, as well as for Next.js, with as little hassle for library users as possible?

When using the library in SPAs (e.g. built with Vite), it just works, but we've come across quite a few issues we've had to solve to make it "Next.js-compatible".

@pawelblaszczyk5
Copy link

If importing the "client only" package makes it so that library users themselves have to wrap all package-components in their own "use client"-components, then that seems overly complicated, and something that needs an easier solution.

The points is that you can’t just wrap all components from your library with “use client”. That’s incorrect, whether you do that or the consumer.

AFAIK if your library uses React’s client features and components aren’t created with these additional boundaries restrictions in mind - adding import client-only is the correct solution, despite the additional hassle. I get your point that’s additional friction but I personally think what lacks here mostly is people being aware how this stuff works.

@sebastianvitterso
Copy link
Author

Ok, we'll have to look closer at whether we want to add import "client-only" to some parts of the library (e.g. the components with unserializable props).

However, at the core of the issue (and what started this GitHub-issue) is us not being able to make Table.Body work from our library. And I'm wondering if that's something that just isn't possible server-side at all, or if it's something we can fix by structuring our code differently, or adding clever "use client" directives in the correct places.

Hopefully you can see an issue with our current approach when it comes to this as well. This is what my examples are about, mostly.

@pawelblaszczyk5
Copy link

Ok, we'll have to look closer at whether we want to add import "client-only" to some parts of the library (e.g. the components with unserializable props).

However, at the core of the issue (and what started this GitHub-issue) is us not being able to make Table.Body work from our library. And I'm wondering if that's something that just isn't possible server-side at all, or if it's something we can fix by structuring our code differently, or adding clever "use client" directives in the correct places.

Hopefully you can see an issue with our current approach when it comes to this as well. This is what my examples are about, mostly.

I think that could be possible if instead of mutating component and doing Component.X = X you try using export * as Component but that also has some limitations. Check out this article - https://ivicabatinic.from.hr/posts/multipart-namespace-components-addressing-rsc-and-dot-notation-issues

@sebastianvitterso
Copy link
Author

Alright thank you! I'll close this issue as resolved, and we'll have to have a discussion on how to best support Nextjs in EDS.

I suppose simply exposing each components seperately should work fine as well (TableBody instead of Table.Body).

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

No branches or pull requests

2 participants