-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
[core] Remove 'use client' from index files #331
Conversation
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". |
Sorry, I don't quite understand what you're proposing above.
We must add "use client" in our component files as we use APIs not supported by RSC (context, event handlers) |
The point I was trying to make is that, for example:
Doesn't have "use client" but it is seems that it should. Removing
at the same time. But I don't know, it might not be that important: vercel/next.js#64467 (comment) 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. |
Is this only necessary when using the star ( In our new API, we export each component individually. Only the types use the star (might need
|
Netlify deploy preview |
66792d0
to
694612f
Compare
I feel the same. Technically it's the component (or hook) that must be ran on the client. |
e59251e
to
7f66e69
Compare
@atomiks, I'd appreciate a review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 it looks like this will help with Next.js integration and ESM 👍 mui/material-ui#42750 (comment)
packages/mui-base/src/AlertDialog/Backdrop/AlertDialogBackdrop.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, but I suppose there shouldn't be a gap between 'use client'
as Olivier mentioned
Cool cleanup 👌
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. |
I personally prefer more whitespace, but it's not a dealbreaker. |
Closes #330.