Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

examples(with-mdx): update to MDX 3 #62503

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

remcohaszing
Copy link
Contributor

What?

Update the with-mdx example to MDX 3.

Why?

The example was 2 major versions behind.

How?

By updating the version in package.json. Also the @mdx-js/react dependency was removed, as it doesn’t work with the app directory. Users should use mdx-components.tsx instead.

Refs #59693

The example was 2 major versions behind.

The `@mdx-js/react` dependency was removed, as it doesn’t work with the
app directory. Users should use `mdx-components.tsx` instead.

Refs vercel#59693
@remcohaszing remcohaszing requested review from a team as code owners February 25, 2024 12:01
@remcohaszing remcohaszing requested review from timeyoutakeit and leerob and removed request for a team February 25, 2024 12:01
@ijjk ijjk added the examples Issue was opened via the examples template. label Feb 25, 2024
@ijjk
Copy link
Member

ijjk commented Feb 25, 2024

Allow CI Workflow Run

  • approve CI run for commit: e4079f9

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@@ -6,8 +6,7 @@
"start": "next start"
},
"dependencies": {
"@mdx-js/loader": "^1.5.1",
"@mdx-js/react": "^1.6.18",
Copy link
Member

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

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.

Copy link
Contributor Author

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 the mdx-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.

Copy link
Member

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.

Copy link
Contributor Author

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.

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.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting... good point! TIL

@karlhorky
Copy link
Contributor

@samcx would it be possible to get another review of this PR?

Copy link
Member

@samcx samcx left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a PR!

:lgtm:

@samcx samcx merged commit 689e4b8 into vercel:canary Jun 27, 2024
34 checks passed
@remcohaszing remcohaszing deleted the update-with-mdx-example branch June 27, 2024 10:05
ztanner added a commit that referenced this pull request Jun 27, 2024
ztanner added a commit that referenced this pull request Jun 27, 2024
ztanner added a commit that referenced this pull request Jun 27, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
examples Issue was opened via the examples template. locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants