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

[core] Remove 'use client' from index files and useAutocomplete reexport #41956

Merged
merged 7 commits into from
Aug 28, 2024

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Apr 18, 2024

Remove 'use client' from:

  • index files
  • useAutocomplete material-ui reexport file. This required a minor modification of the buildRsc script.

This is to avoid an issue with Next.js that surfaced in #41596:

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

Also suggested in #40358 (comment).
There's no use of 'use client' in index files.

@DiegoAndai DiegoAndai added the core Infrastructure work going on behind the scenes label Apr 18, 2024
@DiegoAndai DiegoAndai self-assigned this Apr 18, 2024
@mui-bot
Copy link

mui-bot commented Apr 18, 2024

Netlify deploy preview

https://deploy-preview-41956--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 32d0f0a

@DiegoAndai DiegoAndai changed the title [core] Remove 'use client' from index files and useAutocomplete reexport [core] Remove 'use client' from index files and useAutocomplete reexport Apr 18, 2024
@DiegoAndai DiegoAndai force-pushed the remove-use-client-from-index branch from a032721 to 6486ba7 Compare April 18, 2024 21:01
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 19, 2024

Nice, a continuation of #40663.

I'm confused to why Next.js can't properly handle "use client" in barrel files, it sounds off. But in any cases, it makes sense to me to toggle between client and server bundles at the lowest possible level.

I proposed the same change for Base UI: mui/base-ui#330

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mj12albert was there a reason for adding the 'use client' directive in the index files in the first place. I am ok with the changes 👌

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 14, 2024
@Janpot
Copy link
Member

Janpot commented Aug 28, 2024

@DiegoAndai I'm adding the following to the no-restricted-syntax rule in eslint to prevent the pattern from re-entering in the future:

{
  message: "The 'use client' pragma can't be used with export * in the same module. This is not supported by Next.js.",
  selector: 'ExpressionStatement[expression.value="use client"] ~ ExportAllDeclaration',
},

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 28, 2024
.eslintignore Outdated Show resolved Hide resolved
Co-authored-by: Olivier Tassinari <[email protected]>
Signed-off-by: Jan Potoms <[email protected]>
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good from my POV

@DiegoAndai I merged master, added the lint rule and updated a few files that it caught

@DiegoAndai
Copy link
Member Author

Thanks @Janpot 😊

@DiegoAndai DiegoAndai merged commit f71b23a into mui:master Aug 28, 2024
19 checks passed
@DiegoAndai DiegoAndai deleted the remove-use-client-from-index branch August 28, 2024 16:28
@@ -27,6 +27,7 @@ const PROJECTS: Project[] = [
'packages/mui-material/src/PigmentContainer/PigmentContainer.tsx', // RSC compatible
'packages/mui-material/src/PigmentGrid/PigmentGrid.tsx', // RSC compatible
'packages/mui-material/src/PigmentStack/PigmentStack.tsx', // RSC compatible
'packages/mui-material/src/useAutocomplete/useAutocomplete.js', // RSC compatible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DiegoAndai This file still has the "use client" 🤔

Copy link
Member

@oliviertassinari oliviertassinari Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, It makes sense to me to remove

from all hooks. From a quick test that I could don't with Next.js. If we add the "use client" in any function, we get no clear error when used wrong. Next.js only gives you an empty import on the server bundle when trying to import from the client bundle.

If confirmed, we should update packages/rsc-builder/buildRsc.ts to reflect this and have an eslint error that crash when we try.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Next.js only starts erroring on this once we add the exports field to package.json. I have the feeling it's not as strict with commonjs. That's why you currently can't reproduce.
  • We have a lint rule. It checks whether 'use client' and export * are used in the same file. This is the pattern that Next.js doesn't like and thus we should avoid. The fact that we had a lot of those in index files is a coincidence.
  • imo buildRsc is not the optimal approach to this problem. We should just add 'use client' where it's necessary. I think it would be better to write a test instead that imports all our components in an RSC so that we don't forget this. Just in general, our preference for solving these sorts of problems should be linting/testing first, before code generation.

This comment was marked as outdated.

Copy link
Member

@oliviertassinari oliviertassinari Sep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, my previous message don't holp up when looking at it in more depth: #43778. I only considered Next.js@latest but they had a regression in the DX, they will fix it.

TL;DR we should add "use client" in any file that relies on client-side React API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes nextjs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants