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

Using mdx in @next #5149

Closed
eckdanny-osi opened this issue Sep 28, 2018 · 12 comments
Closed

Using mdx in @next #5149

eckdanny-osi opened this issue Sep 28, 2018 · 12 comments
Labels

Comments

@eckdanny-osi
Copy link

@eckdanny-osi eckdanny-osi commented Sep 28, 2018

I think many CRA users like myself are using mdx-js/mdx. The mdx docs for CRA setup use create-react-app-rewired, but I've managed just fine in CRA v1.x without ejecting or rewiring by inlining webpack loader and eslint overrides:

/* eslint-disable import/no-webpack-loader-syntax */
import Content from '!babel-loader!@mdx-js/loader!./Content.mdx';

I tried the same course of action in @2.0.0 but its not working for reasons I haven't looked into yet. (I'll leave details in a comment below. Raising at mdx-js may be appropriate.)

THE REASON I'm submitting this issue is to identify MDX usage as a likely reason for ejecting/rewiring in response to @Timer's comment timarney/react-app-rewired#162 (comment). The future support outlook for rewired and missing workaround for CRA@next is unfortunate for MDX users.

@eckdanny-osi
Copy link
Author

@eckdanny-osi eckdanny-osi commented Sep 28, 2018

The err I mentioned in OP:

Failed to compile.

./src/Content.mdx)
Syntax error: <cra@next>/src/Content.mdx: Unexpected token (6:43)

  4 |
  5 |
> 6 | export default ({components, ...props}) => <MDXTag name="wrapper"  components={components}><MDXTag name="p" components={components}>{`hi`}</MDXTag></MDXTag>
    |                                            ^
  7 |

Not sure what's root cause... Odd to me the same inline-loader syntax in app code and same version of mdx-js/loader/mdx-js/mdx behaves differently in my CRA v1.x and CRA v2.0.0

@Timer Timer added this to the 2.0.0 milestone Sep 28, 2018
@Timer
Copy link
Collaborator

@Timer Timer commented Sep 28, 2018

@gaearon should we support this natively or via a babel macro?

If natively, when imported via import Content from './Content.mdx'; or only when import { ReactComponent as Content } from './Content.mdx';?

Macro would work like so:

import { mdx } from 'mdx.macro'
const Content = mdx('./Content.mdx');
@gaearon
Copy link
Member

@gaearon gaearon commented Sep 28, 2018

Macro is nice if it means we don’t have to include MDX compiler by default.

@gaearon gaearon mentioned this issue Sep 28, 2018
19 of 25 tasks complete
@eckdanny-osi
Copy link
Author

@eckdanny-osi eckdanny-osi commented Sep 28, 2018

Tnx guys, I hadn't yet been introduced to babel-plugin-macros (which is amazing).

The usage hinted at above and expectation that users be responsible for satisfying a dep on mdx-js/mdx compiler seem perfectly appropriate to me.

next steps: shall I open an an issue with the @mdx-js/* repo maintainers to seeking provision of a @mdx-js/macro or mdx.macro package? (If the tech impl sol'n space is not in CRA then maybe this issue can be closed.)

I'm happy CRA@next is providing an escape hatch for these sorts of things.

@Timer
Copy link
Collaborator

@Timer Timer commented Sep 29, 2018

We should ask for a @mdx-js/macro or mdx.macro package!

@sw-yx
Copy link
Contributor

@sw-yx sw-yx commented Oct 6, 2018

hi! just browsing. i am not sure i understand what exactly the macro would solve for you since @eckdanny-osi's original problem was that the webpack loader inlining was broken. the macro won't be able to do anything to fix that. maybe we can open an issue over at @mdx-js and follow on from there.

@Timer
Copy link
Collaborator

@Timer Timer commented Oct 6, 2018

original problem was that the webpack loader inlining was broken

Using the method displayed in OP is not supported by CRA and never expected to work.
MDX support will only be allowed via a Babel Macro, thus the suggestion.

@eckdanny-osi
Copy link
Author

@eckdanny-osi eckdanny-osi commented Oct 27, 2018

hey guys. sry haven't checked back in on this convo in a while.

My project is back to using webpack's inline-loader syntax w/ react-scripts@2.0.4. Only having *just discovered babel-plugin-macros when i 1st posted, I had hoped they would be enough escape hatch, but @sw-yx is right. My project could impl some one-off macros using @mdx-js/mdx (e.g.; parse and compile small snippets to JSX), but what we really need/want is to tap into the loader chain.

We were on react-scripts-cssmodules before CRA@2 shipped. I guess I imagined the path to support MDX in CRA and the developer end-user experience would be similar to the sass support story. (webpack plumbing is there for us, but we must add the compiler to use it like the CLI and docs say for sass)

If my project hits more walls we can't circumnavigate with inline-loader syntax, we'll likely eject or maintain a fork. Don't know how unique my situation is... am curious to what degree CRA users want mdx support tho

@Timer Timer removed this from the 2.x milestone Nov 2, 2018
@stale
Copy link

@stale stale bot commented Dec 2, 2018

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 2, 2018
@jamesknelson
Copy link
Contributor

@jamesknelson jamesknelson commented Dec 7, 2018

One issue with supporting MDX in the same way that SCSS is supported is that it's rare that you'll want to use vanilla MDX. At minimum, you're probably going to want to add plugins to improve typography, add emoji support, and handle front matter -- but you can go further and do things like add id props to titles for deep linking, generate a table of contents, etc.

I've tried putting together an MDX macro with all this, but can't find a way to get recompile-on-save working, which is a bummer.

I do have MDX working with CRA@2 though, using import('!babel-loader!mdx-loader!./tutorial/make-a-blog.md') and the following .babelrc:

{
  "presets": ["react-app"]
}

Even if this isn't supported, I feel like it's probably the most pragmatic approach atm.

@stale
Copy link

@stale stale bot commented Jan 6, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 6, 2019
@stale
Copy link

@stale stale bot commented Jan 12, 2019

This issue has been automatically closed because it has not had any recent activity. If you have a question or comment, please open a new issue.

@stale stale bot closed this Jan 12, 2019
@lock lock bot locked and limited conversation to collaborators Jan 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.