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

docs(mdx): add a dynamic imports section for App Router #73466

Merged
merged 11 commits into from
Dec 14, 2024

Conversation

samcx
Copy link
Member

@samcx samcx commented Dec 3, 2024

Why?

Adding a section dedicated to dynamically importing MDX files inside pages for App Router.

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. Documentation Related to Next.js' official documentation. labels Dec 3, 2024
Copy link
Member Author

samcx commented Dec 3, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@samcx samcx requested a review from delbaoliveira December 3, 2024 08:33
@samcx samcx marked this pull request as ready for review December 3, 2024 08:34
@samcx
Copy link
Member Author

samcx commented Dec 3, 2024

cc @karlhorky I don't think this is as seamless (probably possible) with Pages Router since pages can't be async in Pages Router, so opted to just stick to adding this for App Router.

@samcx samcx force-pushed the sam/docs/mdx-dynamic-imports branch from 839a24f to 774554d Compare December 3, 2024 08:35
Copy link
Contributor

@karlhorky karlhorky left a comment

Choose a reason for hiding this comment

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

I would suggest removing / reworking the "Remote MDX" section, at least for App Router:

This section introduces secondary terminology "remote MDX" and recommends a 3rd-party library, whereas these capabilities are possible with only @next/mdx

@samcx
Copy link
Member Author

samcx commented Dec 3, 2024

@karlhorky I'm not sure if this is the same capabilities—the Remote MDX example is using raw Markdown text from fetch, whereas my example is importing a markdown file as a Component.

We could probably figure out a way to fetch Markdown text and make it usable, but it seems more complicated to implement than just <MDXRemote source={markdown} />.

Maybe we just remove this line from the Remote MDX section → " content stored in a separate local folder"?

@samcx samcx requested a review from karlhorky December 3, 2024 09:07
@karlhorky
Copy link
Contributor

karlhorky commented Dec 3, 2024

I'm not sure if this is the same capabilities—the Remote MDX example is using raw Markdown text, whereas my example is importing a markdown file as a Component. If we fetched MDX text, how would we pa

With this part "If we fetched MDX text, how would we pa", I guess you mean "how would we parse and compile the MDX"

Maybe @next/mdx won't completely take over all capabilities of next-mdx-remote, but maybe your next comment also has a suggestion of how to deal with that:

Oh I guess we have this section → nextjs.org/docs/app/building-your-application/configuring/mdx#deep-dive-how-do-you-transform-markdown-into-html

That's a good point, we could potentially suggest that.

Maybe we just remove this line from the Remote MDX section → " content stored in a separate local folder"?

In case you think there's a high value of keeping the "Remote MDX" section, this would also be a possibility:

Eg. remove the parts about local files, which overlap with @next/mdx capabilities with await import().

Maybe also add more language about this being an "alternative" and more emphasis that it's "third party". Could also be moved further down in the docs.

@samcx
Copy link
Member Author

samcx commented Dec 3, 2024

@karlhorky Sounds good! It's late here (the next day haha) so I will finish later today!

@karlhorky
Copy link
Contributor

Also, if the following people have time, would also request a PR review from you as well:

@remcohaszing
Copy link
Contributor

<MDXRemote source={markdown} />

This looks suspicious. markdown is a string. Is the source markdown or MDX? Usually this is inferred from the file name, defaulting to MDX. But no file name is given here and the variable is named markdown.

Copy link
Contributor

@wesbos wesbos left a comment

Choose a reason for hiding this comment

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

couple clarifications needed as you can run into issues here

@samcx samcx force-pushed the sam/docs/mdx-dynamic-imports branch from 0befb1d to eba4bdd Compare December 3, 2024 21:16
@samcx
Copy link
Member Author

samcx commented Dec 3, 2024

<MDXRemote source={markdown} />

This looks suspicious. markdown is a string. Is the source markdown or MDX? Usually this is inferred from the file name, defaulting to MDX. But no file name is given here and the variable is named markdown.

@remcohaszing I think markdown is actually correct → https://github.com/hashicorp/next-mdx-remote?tab=readme-ov-file#react-server-components-rsc--nextjs-app-directory-support

@samcx samcx requested review from wesbos, leerob and karlhorky December 3, 2024 22:08
@samcx samcx force-pushed the sam/docs/mdx-dynamic-imports branch from 5703760 to 985e9b0 Compare December 6, 2024 22:54
@samcx samcx force-pushed the sam/docs/mdx-dynamic-imports branch from 985e9b0 to 5187c6e Compare December 13, 2024 23:15
@samcx samcx requested a review from leerob December 13, 2024 23:15
@samcx samcx force-pushed the sam/docs/mdx-dynamic-imports branch from 5187c6e to a56956e Compare December 13, 2024 23:15
@samcx samcx requested a review from delbaoliveira December 13, 2024 23:15
@leerob leerob merged commit 21fa773 into canary Dec 14, 2024
53 checks passed
@leerob leerob deleted the sam/docs/mdx-dynamic-imports branch December 14, 2024 00:30
@karlhorky
Copy link
Contributor

Thanks everyone for getting this in! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
created-by: Next.js team PRs by the Next.js team. Documentation Related to Next.js' official documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants