Skip to content

[code-infra] Add "use client" directive to files with React APIs #45036

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 21, 2025
Merged

[code-infra] Add "use client" directive to files with React APIs #45036

merged 5 commits into from
Jan 21, 2025

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jan 16, 2025 •
edited
Loading

This otherwise breaks react server components in Next.js when moving to full ESM.

copilot wrote me a eslint plugin that detects those and autofixes. we can't add them to index files

For reference: vercel/next.js#75128

@Janpot Janpot added the scope: code-infra Changes related to the core-infra product. label Jan 16, 2025
@mui-bot
Copy link

mui-bot commented Jan 16, 2025 •
edited
Loading

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 30b6c8f

@Janpot Janpot marked this pull request as ready for review January 16, 2025 18:39
@Janpot Janpot requested a review from DiegoAndai January 16, 2025 19:05
DiegoAndai
DiegoAndai reviewed Jan 16, 2025
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Hey @Janpot,

What do you mean with:

we can't add them to index files

packages/mui-material/src/Autocomplete/Autocomplete.test.js Outdated
@@ -1,3 +1,4 @@
'use client';
Copy link
Member

Choose a reason for hiding this comment

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

Should we exclude test files?

@Janpot
Copy link
Member Author

Janpot commented Jan 16, 2025

Hey @Janpot,

What do you mean with:

we can't add them to index files

Didn't we uncover that in #41956?

@DiegoAndai
Copy link
Member

Didn't we uncover that in #41956?

I didn't know if you were referring to that or the eslint plugin.

The reason is to avoid 'use client' and export * in the same file, which causes the error described in #41956. Could we add another eslint plugin to cover this? Let me know if it's out of scope, and we can tackle it in a separate PR.

@Janpot
Copy link
Member Author

Janpot commented Jan 17, 2025 •
edited
Loading

Could we add another eslint plugin to cover this?

🙂 It's already covered with the no-restricted-syntax. It seemed the use-case of this PR is a little too complex for this rule. I couldn't make it work.

material-ui/.eslintrc.js

Lines 254 to 258 in 820a76c

{
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',
},

@Janpot Janpot mentioned this pull request Jan 20, 2025
Merged
@Janpot Janpot merged commit 4f872d6 into mui:master Jan 21, 2025
19 checks passed
@Janpot Janpot mentioned this pull request Jan 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiegoAndai DiegoAndai DiegoAndai approved these changes

Assignees
No one assigned
Labels
scope: code-infra Changes related to the core-infra product.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Janpot @mui-bot @DiegoAndai