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

use mdx-loader instead of mdx.macro #589

Closed
wants to merge 3 commits into from
Closed

use mdx-loader instead of mdx.macro #589

wants to merge 3 commits into from

Conversation

adueck
Copy link

@adueck adueck commented May 30, 2019

I think it would be better to suggest using mdx-loader with create-react-app because:

  • Live reloading works! 🎉 No more known bug necessary.
  • Now mdx-loader supports v. 1 of @mdx-js/mdx, so crucial things like JSX in the middle of markdown copmonents works. (See this recently closed issue.). mdx.macro does not support the latest @mdx-js/mdx
  • This ends up being simpler and cleaner to use in the React code, in my opinion

I've updated both the docs page and the full example.

@vercel
Copy link

vercel bot commented May 30, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://mdx-git-fork-adueck-master.mdx.now.sh

@vercel vercel bot temporarily deployed to staging May 30, 2019 13:04 Inactive
@vercel vercel bot temporarily deployed to staging May 30, 2019 13:10 Inactive
@vercel vercel bot temporarily deployed to staging May 30, 2019 13:13 Inactive
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @adueck!

I'd lean toward keeping the Babel macro as the recommendation and example.
The React team wants to avoid custom loader syntax, as evidenced by eslint import/no-webpack-loader-syntax being on by default.
While not having live reloading today is less than ideal, babel has a proposed solution babel/babel#8497 that will enable webpack, create react app, and others to handle this without workarounds.
I'd rather wait for the full solution than ask adopters to apply a work around that they'll want to rollback at a later date.

@adueck
Copy link
Author

adueck commented Jun 4, 2019

@ChristianMurphy Yes I see your point, fair enough. Just a couple of things to consider (if it affects your decision at all)

The README for mdx-loader points out that

You can also import documents dynamically using the proposed import() syntax and React.lazy(), without messing with linter config:

const MyDocument = React.lazy(() => import('!babel-loader!mdx-loader!../pages/index.md'))

But I guess that still is custom loader syntax.

I also think it's important to remember that mdx.macro does not work with v. 1 of mdx, and this probably will not change anytime soon, based on this comment from the author.

Anyways ultimately your call of course 😄. I've chosen to use mdx-loader personally, because it was really really crucial for me to have:

  • mdx v. 1 support (for midline jsx!)
  • live reloading

Maybe it's best just to let people know that this mdx-loader is also an option?

@ChristianMurphy
Copy link
Member

You can also import documents dynamically using the proposed import() syntax and React.lazy(), without messing with linter config

That appears this is due to no-webpack-loader-syntax not being setup to check dynamic imports yet.
Expect that to change in a future release.

I also think it's important to remember that mdx.macro does not work with v. 1 of mdx, and this probably will not change anytime soon, based on this comment from the author.

🤔 the babel macro could be something worth considering moving into core, rather than a community plugin.
Thoughts @mdx-js/core?

Maybe it's best just to let people know that this mdx-loader is also an option?

Definitely! letting folks know it's an option would be nice.

@johno
Copy link
Member

johno commented Jun 5, 2019

🤔 the babel macro could be something worth considering moving into core, rather than a community plugin

Yeah, definitely. Would you be interested in adding mdx.macro to this monorepo @jamesknelson?

Maybe it's best just to let people know that this mdx-loader is also an option?

Definitely! letting folks know it's an option would be nice.

This would be a great guide!

@jamesknelson
Copy link

jamesknelson commented Jun 6, 2019

Sure, I'd be happy to transfer mdx.macro to the mdx-js monorepo. It'd make sense to be able to write inline MDX using a macro.

It really needs a complete rewrite though, and that rewrite should probably limit it to inline MDX. It's not worth the effort to make it handle external files, as loading external files with babel-plugin-macros is just not a good idea. Even if babel/babel#8497 is eventually implemented, it would still require each macro to handle all the file loading edge cases by itself. Why bother when Webpack was actually built for this, and does a perfectly good job already?

I mean, compare these two hypothetical syntaxes:

// babel-macros
import mdx from 'mdx.macro'
let Doc = React.lazy(() => mdx.import('./doc.mdx'))

// webpack loaders
let Doc = React.lazy(() => import('!mdx-loader!./doc.mdx'))

Neither of these are plain JavaScript -- they both require a build step. Sure, one is allowed by the CRA linter, but the other actually works. There's nothing objectively better about the macros syntax, other than the fact that some lint rules have been put in place that make the other one uglier. And if we're going to be fair, the loader syntax is actually far nicer when you have an MDX loader set up in the Webpack config.

So I guess what I'm saying is I'd be more than happy to transfer mdx.macro. It makes a lot of sense to have an option to write inline mdx using macros. However, for importing mdx files, we really should recommend that people use a Webpack loader -- regardless of what the CRA team says.

@adueck
Copy link
Author

adueck commented Jun 6, 2019

However, for importing mdx files, we really should recommend that people use a Webpack loader -- regardless of what the CRA team says.

@jamesknelson So by a Webpack loader you mean mdx-loader then?

It seems that a lot of, if not most use cases would be importing .mdx files.

@jamesknelson
Copy link

@adueck There's also @mdx-js/loader, which is the official loader and seems to get much more use than mdx-loader.

@adueck
Copy link
Author

adueck commented Jun 8, 2019

@jamesknelson But from my understaing @mdx-js/loader doesn't have id slugs on headings, table of contents, front matter parsing etc. included in there that all has to be added by the user, am I right? If so all that stuff is what makes your mdx-loader such a great go-to. 👍

@Lillebo
Copy link

Lillebo commented Feb 2, 2020

@adueck Are there any special instructions for how to use this with require.context? I'm currently trying this:

const mdxContext = require.context('!babel-loader!mdx-loader!../../../../packages', true, /(\w+)\.(\w+)\.(mdx?)/);

And I'm getting a whole bunch of "Unexpected token" errors from babel-loader 😕

@adueck
Copy link
Author

adueck commented Feb 3, 2020

@Lillebo Sorry, I haven't used it with require.context so I don't know. I've only imported individual files with simple lines like

import * as myPage from '!babel-loader!mdx-loader!./my-page.mdx';

@ChristianMurphy
Copy link
Member

@adueck looks like a merge conflict has cropped up, any chance this could be rebased and updated to use @mdx-js/loader?

@wooorm
Copy link
Member

wooorm commented Nov 13, 2020

@adueck Friendly ping! 🛎😊

@thediveo
Copy link

mdx.macro has become totally detached from currently documented mdx functionality. Please merge this PR so users don't find out the hard way that the documentation is recommending an unmaintained and outdated pre-1.0 installation.

@wooorm
Copy link
Member

wooorm commented Dec 18, 2020

@thediveo As this PR seems stalled, a new PR would be appreciated!

@thediveo
Copy link

thediveo commented Dec 18, 2020

Don't see how a fresh PR will get better treatment than the original one? The project maintainers let this PR decomposing already for, wait, 2 years if I'm not mistaken. I can supply a PR, but only if the maintainers signal "go on".

I noticed that I didn't need to create any babel-related resource file, for a current cra it seems to work out-of-the-box.

@wooorm
Copy link
Member

wooorm commented Dec 18, 2020

I'm a maintainer and I'm "signalling" that.

wooorm added a commit that referenced this pull request Dec 28, 2020
Create React App is the most looked at resource by users here on GitHub.
But it’s suggesting an old, unmaintained, and buggy way to use MDX.

This instead updates the guide to use our maintained projects, without
having to eject from CRA.

As CRA itself is an ever-changing “init” tool, which can support MDX by
following a couple steps, I don’t think it’s wise to have an example in
the project: we want folks to do `npx create-react-app ...`, not clone
our custom example.

Not having CRA checked in also makes for a faster `yarn install`.

Perhaps developing our own [CRA
template](https://create-react-app.dev/docs/custom-templates) might be
nice for the future, but for now I’ve kept it at an up to date and
working guide.

Related to GH-1015.
Related to GH-1388.

Closes GH-365.
Closes GH-589.
@wooorm wooorm mentioned this pull request Dec 28, 2020
wooorm added a commit that referenced this pull request Dec 28, 2020
Create React App is the most looked at resource by users here on GitHub.
But it’s suggesting an old, unmaintained, and buggy way to use MDX.

This instead updates the guide to use our maintained projects, without
having to eject from CRA.

As CRA itself is an ever-changing “init” tool, which can support MDX by
following a couple steps, I don’t think it’s wise to have an example in
the project: we want folks to do `npx create-react-app ...`, not clone
our custom example.

Not having CRA checked in also makes for a faster `yarn install`.

Perhaps developing our own [CRA
template](https://create-react-app.dev/docs/custom-templates) might be
nice for the future, but for now I’ve kept it at an up to date and
working guide.

Related to GH-1015.
Related to GH-1388.

Closes GH-365.
Closes GH-589.
@wooorm wooorm closed this in #1422 Dec 29, 2020
wooorm added a commit that referenced this pull request Dec 29, 2020
Create React App is the most looked at resource by users here on GitHub.
But it’s suggesting an old, unmaintained, and buggy way to use MDX.

This instead updates the guide to use our maintained projects, without
having to eject from CRA.

As CRA itself is an ever-changing “init” tool, which can support MDX by
following a couple steps, I don’t think it’s wise to have an example in
the project: we want folks to do `npx create-react-app ...`, not clone
our custom example.

Not having CRA checked in also makes for a faster `yarn install`.

Perhaps developing our own [CRA
template](https://create-react-app.dev/docs/custom-templates) might be
nice for the future, but for now I’ve kept it at an up to date and
working guide.

Related to GH-1015.
Related to GH-1388.

Closes GH-365.
Closes GH-589.
Closes GH-1422.

Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com>
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.

7 participants