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
add use client directive to react package #2443
Conversation
Frameworks like Next.js default to `@mdx-js/react` if no `mdx-components` file is present. When this happens an error is thrown due to `useContext` in `useMDXComponents` and it's not apparent what's going on. This fixes this default use case in Next.js where overriding MDX components is not needed. Signed-off-by: Travis Arnold <ftntravis@gmail.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2443 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 2693 2694 +1
Branches 2 2
=========================================
+ Hits 2693 2694 +1 ☔ View full report in Codecov by Sentry. |
Welcome @souporserious! 👋
I'm not opposed to this. Though I am a bit cautious, directives are still a canary feature of React (as of 2024-02-15) and subject to change ahead of the React 19 release.
I do see some notes about context in the canary docs https://react.dev/reference/react/use-client#using-third-party-libraries It is also worth noting, while using With that in mind, I wonder if it would be worth having a context-free server-side render-able version as well as a context-full client-only version. 🤔 /cc @mdx-js/core thoughts? |
Totally understand wanting to approach with caution. It seems the client directive is stable so I think this should be safe, but it's close to the 19 release so no harm in waiting. Per the error message, it's clear from react and points to adding Separate builds could work nicely as well where |
I think this is a good change for RSC in general, not necessarily for Next.js. They are better off accepting vercel/next.js#59693 and applying the additional suggestion there. Did you test this change to make sure it works? |
What is the "harm" that's being fixed here? What happens without this change, what happens with, what are the alternatives? Is our react project really a client file/component? Not against, I just don't really know what's happening and why! |
@wooorm this is the error in Next.js that is thrown without adding an This PR was an attempt to fix that, but after making an example here it looks like there's this additional Next.js error so it doesn't seem adding
Good point, probably best to let the author choose where the boundary is. @remcohaszing yeah, this does seem to be more of a Next.js related issue so I'll close this PR in favor the PR you linked. Providing a default |
Initial checklist
Description of changes
Frameworks like the Next.js MDX plugin default to
@mdx-js/react
if nomdx-components
file is present. When this happens, an error is thrown due touseContext
inuseMDXComponents
and it's not apparent what's going on. This fixes this default use case in Next.js when overriding MDX components is not needed.