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.73k stars 1.09k forks source link

mergeProps doesn't work for Server Components #6592

Open BorisZubchenko opened 3 months ago

BorisZubchenko commented 3 months ago

Provide a general summary of the issue here

Using mergeProps in server components gives the following error:

Error: createContext only works in Client Components. Add the "use client" directive at the top of the file to use it.

The only solution is to mark component with 'use-client' directive.

๐Ÿค” Expected Behavior?

Being able to use the function in server components.

๐Ÿ˜ฏ Current Behavior

Can't be used without client components boundary.

๐Ÿ’ Possible Solution

Function's implementation is pretty straightforward. It uses mergeIds from useId.ts. This function does not use any "client" features, but the file itself imports them. The solution I see is to extract idsUpdaterMap and mergeIds to another file so it doesn't import any hooks/contexts.

๐Ÿ”ฆ Context

No response

๐Ÿ–ฅ๏ธ Steps to Reproduce

https://stackblitz.com/edit/stackblitz-starters-zsr6u8?file=app%2Fpage.tsx

Here I just import mergeProps to a server component. This is enough to raise the error.

Version

@react-aria/utils 3.24.1

What browsers are you seeing the problem on?

Other

If other, please specify.

No response

What operating system are you using?

Windows

๐Ÿงข Your Company/Team

No response

๐Ÿ•ท Tracking Issue

No response

snowystinger commented 3 months ago

Seems reasonable

philwolstenholme commented 1 month ago

This would be great, I love using mergeProps but it's a shame to have to mark each component that uses it as a client component.

For some reason I thought it used some client only features (Context) โ€“ I'd have raised an issue earlier if I had known it was just due to some neighbouring imports from the same utils file!

snowystinger commented 1 month ago

Question for this, I don't quite know enough about client only yet. Is it enough to move them into their own file within the package and still expose them through the same index file as the rest? Or would they need to be in their own package?

BorisZubchenko commented 1 month ago

@snowystinger I've just created a reproduction at StackBlitz and it seems that re-exporting does not solve the problem.

I think it can still be exposed through the index file of a package, however, one should be able to import it directly. So having a module that has the mergeProps and that's it.

philwolstenholme commented 1 month ago

@snowystinger I've just created a reproduction at StackBlitz and it seems that re-exporting does not solve the problem.

I think it can still be exposed through the index file of a package, however, one should be able to import it directly. So having a module that has the mergeProps and that's it.

That sounds right to me too :) the barrel export file can still reference it and people not using React Server Components can continue to use that import (so there's no breaking change), but RSC users would benefit from a new import with just mergeProps (and potentially any other helpful utils in the future that don't rely on client side hooks/context etc).

devongovett commented 1 month ago

Question: why do you need mergeProps in a server component when most of what it does relies on client features? For example, merging event handlers and ids relies on client features like events and useState. The only thing left that Object.assign or object spreading doesn't cover is merging classnames with clsx.

BorisZubchenko commented 1 month ago

Consistency:

  1. Sure, functions are not serializable and thus can't be used as props within server components. But thinking if it's a server or a client component is an extra overhead. Not to mention the fact that components are not actually "client" or "server". They are just components. We use the use client directive to provide the boundary for the server.

  2. I also think that the mergeProps might be a factory, so one could provide a class merging strategy. For example, using tailwind-merge. Yes, this is not implemented, but with consistency, it would be easy to add new features like this.

  3. I don't always have clsx installed.