add use client directive to react package

souporserious commented 9 months ago

Initial checklist

[x] I read the support docs
[x] I read the contributing guide
[x] I agree to follow the code of conduct
[x] 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.

vercel[bot] commented 9 months ago

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 commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2443 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 23 23 Lines 2693 2694 +1 Branches 2 2 ========================================= + Hits 2693 2694 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ChristianMurphy commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

I think this is a good change for RSC in general, not necessarily for Next.js. They are better off accepting https://github.com/vercel/next.js/pull/59693 and applying the additional suggestion there.

Did you test this change to make sure it works?

wooorm commented 9 months ago

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 commented 9 months ago

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

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:

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.

mdx-js / mdx