base-ui icon indicating copy to clipboard operation
base-ui copied to clipboard

[core] Remove 'use client' from index files

Open michaldudak opened this issue 1 year ago • 6 comments

Closes #330.

michaldudak avatar Apr 19 '24 17:04 michaldudak

As I understand things, it's possible to use Base UI components without having to add "use client" (as a user of Base UI). For example, https://deploy-preview-331--base-ui.netlify.app/base-ui/react-number-field/ imported by a server component seems to work just fine (@mui/system is heading to work, and React.useId works with server components).

Now, sure, developers would need to add events to make use of those Base UI, so need "use client" in their code. But this might only happen higher up, meaning when developers use the design systems built with Base UI. I suspect that adding "use client" in each component file could help with the DX, delaying the time when developers hit an error that requires them to add "use client".

oliviertassinari avatar Apr 19 '24 22:04 oliviertassinari

Sorry, I don't quite understand what you're proposing above.

I suspect that adding "use client" in each component file could help with the DX

We must add "use client" in our component files as we use APIs not supported by RSC (context, event handlers)

michaldudak avatar Apr 22 '24 07:04 michaldudak

The point I was trying to make is that, for example:

https://github.com/mui/base-ui/blob/43c957dd8d41f78b45cb8d5bb7e915d809298779/packages/mui-base/src/NumberField/NumberFieldGroup.tsx#L1

Doesn't have "use client" but it is seems that it should. Removing

https://github.com/mui/base-ui/blob/43c957dd8d41f78b45cb8d5bb7e915d809298779/packages/mui-base/src/NumberField/index.ts#L1

at the same time.

But I don't know, it might not be that important: https://github.com/vercel/next.js/pull/64467#issuecomment-2067817756 if it's fixed in Next.js latest canary. All in all, 👍 to add use client at the lowest level possible, even if it leads to duplication. It feels clearer, but I can't find strong arguments either way.

oliviertassinari avatar Apr 22 '24 10:04 oliviertassinari

Is this only necessary when using the star (*) export?

In our new API, we export each component individually. Only the types use the star (might need export type *).

Error: It's currently unsupported to use "export *" in a client boundary. Please use named exports instead.

atomiks avatar Apr 22 '24 11:04 atomiks

Netlify deploy preview

https://deploy-preview-331--base-ui.netlify.app/

Generated by :no_entry_sign: dangerJS against 0a3080102d5a36bd91ca0ab91b4a7b71090166d3

mui-bot avatar May 29 '24 11:05 mui-bot

👍 to add use client at the lowest level possible, even if it leads to duplication. It feels clearer, but I can't find strong arguments either way.

I feel the same. Technically it's the component (or hook) that must be ran on the client. I updated the PR.

michaldudak avatar May 29 '24 12:05 michaldudak

@atomiks, I'd appreciate a review

michaldudak avatar Sep 04 '24 14:09 michaldudak

Cool cleanup 👌

but I suppose there shouldn't be a gap between 'use client' as Olivier mentioned

It's subjective. The rational was to do it a single way through the codebase of MUI. I saw the majority was without blank lines so pushed to default with the majority. I think Albert initially introduced the no blank line path.

Now, if you look at the React docs, it's different https://react.dev/reference/rsc/use-client.

For use strict, there is no consistency in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode. It's also a mix in https://github.com/search?q=org%3Aradix-ui+%27use+client%27&type=code.

No real preference, as long as it's consistent.

oliviertassinari avatar Sep 18 '24 09:09 oliviertassinari

I personally prefer more whitespace, but it's not a dealbreaker.

michaldudak avatar Sep 18 '24 10:09 michaldudak