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

add use client directive to react package #2443

Closed
wants to merge 1 commit into from

Conversation

souporserious
Copy link

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Frameworks like the Next.js MDX plugin 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 when overriding MDX components is not needed.

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>
Copy link

vercel bot commented Feb 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mdx ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 22, 2024 11:25pm

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (908ff45) to head (aa27aaf).
Report is 10 commits behind head on main.

❗ 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.
📢 Have feedback on the report? Share it here.

@ChristianMurphy
Copy link
Member

Welcome @souporserious! 👋
Thanks for opening a PR and starting a discussion.

This fixes this default use case in Next.js when overriding MDX components is not needed.

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.
https://react.dev/blog/2024/02/15/react-labs-what-we-have-been-working-on-february-2024#new-features-in-react-canary
I want to avoid mdx having to release multiple major versions trying to offer a stable support (in MDX) for an unstable API (in React and Next).
If we add this, I feel like there may need to be some notes about the experimental nature and how this is not covered by SemVer in mdx

When this happens, an error is thrown due to useContext in useMDXComponents and it's not apparent what's going on

I do see some notes about context in the canary docs https://react.dev/reference/react/use-client#using-third-party-libraries
It would be nice if react itself provided better error/warning/info messages to offer guidance, since tracking features which are and are not supported server side will be a react ecosystem wide issue, not just an MDX specific one.

It is also worth noting, while using context is supported, it is generally not recommended.
Passing a components prop directly is the preferred method of adding custom components.

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?

@souporserious
Copy link
Author

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 use client since @mdx/react is trying to render useContext in a Server Component. However, it's awkward when setting up MDX in Next.js since it's not apparent why that happens and how to fix it. Next.js could have a better error or add the mdx-components.jsx file automatically which I think could help. What's nice about adding the directive is it removes the requirement of adding an empty mdx-components.jsx file.

Separate builds could work nicely as well where @next/mdx could import from @mdx-js/react/server.

@remcohaszing
Copy link
Member

remcohaszing commented Feb 24, 2024

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?

@wooorm
Copy link
Member

wooorm commented Feb 24, 2024

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!

@souporserious
Copy link
Author

@wooorm this is the error in Next.js that is thrown without adding an mdx-components file since it defaults to @mdx-js/react:

image

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 use client is a proper fix:

image

Is our react project really a client file/component?

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 mdx-components implementation would be more fitting to address the issue here. It seems @mdx-js/react shouldn't be loaded at all since it doesn't seem to ever work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants