Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a separate example for app router, should this dep remain?
https://github.com/vercel/next.js/tree/canary/examples/app-dir-mdx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it's being covered there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit that
@mdx-js/react
offers, is that it allows users to provide components for MDX files for different files in the React tree. IMO this is a niche use case. I think themdx-components
file is a much better solution for Next.js. Kudos to whoever came up with that.@mdx-js/react
works for the pages directory, but not for the app directory. It just crashes with a weird error. I personally found this confusing and I’ve heard so from others too.Regardless of what works or doesn’t work, IMO it’s good to have a single blessed way to use MDX with Next.js. I think it’s hard to explain what the type of router has to do with this.
Using the
mdx-components
file better aligns with the DX after I implement support for provided components in the MDX language server. Just define them once globally, then use them in every MDX file.What is missing from this PR though, is the addition of the
mdx-components
file. I could add it, but even better would be to merge #59693.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: The
mdx-components
file is only for the App Router, not for the Pages Router.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confused me, because I see no special handling depending on the router type in https://github.com/vercel/next.js/blob/canary/packages/next-mdx/index.js. So I just tried it and this is simply not true. The
mdx-components
file works perfectly fine with either type of router.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... good point! TIL