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

jsxFrag should be set with latest version of @babel-plugin-transform-react-jsx #990

Closed
loganmccaul opened this issue Mar 23, 2020 · 13 comments · Fixed by #1394
Closed

jsxFrag should be set with latest version of @babel-plugin-transform-react-jsx #990

loganmccaul opened this issue Mar 23, 2020 · 13 comments · Fixed by #1394
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done

Comments

@loganmccaul
Copy link

loganmccaul commented Mar 23, 2020

Subject of the issue

I'm getting the following error when compiling mdx files that use a React Fragment, <></> on a Gatsby site.

transform-react-jsx: pragma has been set but pragmaFrag has not been set

The issue is because MDX specifies /* @jsx mdx */ as the pragma, but never specifies a pragmaFrag which is now a requirement with [email protected]`

See the note here:
https://babeljs.io/docs/en/next/babel-plugin-transform-react-jsx.html#customizing-with-the-classic-runtime

Your environment

  • OS: Mac OS mojave
  • Packages: @mdx-js/mdx, @mdx-js/parcel-plugin-mdx, gatsby-plugin-mdx (Plus others I'm sure)
  • Env: node 12.14, npm 6.13

Steps to reproduce

Unfortunately the code is private so I can't provide it as an example, but let me know if you want me to recreate in a public repo.

Expected behaviour

MDX should set a jsxFrag value when specifying a custom jsx pragma.

So for example, every time /* @jsx mdx */ is set /* @jsxFrag ... */ should also be set.

Alternatively, babel-plugin-transform-react-jsx should use the default React.Fragment if none is specified.

Related issues:

Let me know if there's a fragment pragma that makes sense and I'd be happy to do a PR.

Actual behaviour

babel-plugin-transform-react-jsx throws an error.

@loganmccaul
Copy link
Author

Based on babel/babel#11321 (comment) it seems like up until babel/babel@748897b#diff-8c3c9c6362e23bd456da3fb129f49a57L10 mdx-js has been relying on React.Fragment being passed in from preset-react. Since that has now been removed, I think a fix would be to add

/* @jsxFrag React.Fragment */

Anywhere where /* @jsx mdx */ is specified.

Let me know if that makes sense and I'd be happy to do a PR.

@nicolo-ribaudo
Copy link

Note: we are restoring the old behavior of not throwing when using the preset, but I recommend enabling the fragment option since we'll re-introduce the error in Babel 8.

@existentialism
Copy link

We released a fix in 7.9.4 🎉!

@wooorm
Copy link
Member

wooorm commented Mar 24, 2020

Thanks all! Those changes should solve this issue, right? Is there something else we should do in MDX?

@existentialism
Copy link

Should be solved, but keep in mind we will remove the default in Babel 8!

@wooorm
Copy link
Member

wooorm commented Mar 24, 2020

Alright, I’ll close this here, and we’ll solve for it when upgrading to babel 8 when it’s out.

Thanks all!

@wooorm wooorm closed this as completed Mar 24, 2020
@wooorm wooorm added 👀 no/external This makes more sense somewhere else and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Mar 24, 2020
@johno
Copy link
Member

johno commented Mar 25, 2020

Should we keep this open, @wooorm, and update the React loader and runtime? We'll just be specifying the current default anyway and that means we won't have to cut a new version to support babel 8 AFAIK, so we'll be more future proof.

Thanks for your hard work @nicolo-ribaudo and @existentialism, we def appreciate you keeping the same behavior and waiting for babel 8 💟🙏

@wooorm
Copy link
Member

wooorm commented Mar 25, 2020

If changing something here means both current and future Babel works, then, yeah! 👍

@wooorm wooorm reopened this Mar 25, 2020
@nicolo-ribaudo
Copy link

Yes, explicitly setting the options should make it work both with Babel 7 and 8!

@johno johno added good first issue 👋 This may be a great place to get started! help wanted 🙏 This could use your insight or help labels Mar 25, 2020
adammockor added a commit to adammockor/mdx that referenced this issue Apr 13, 2020
Replace /* @jsx mdx */ with /* @jsxFrag React.Fragment */
@wooorm wooorm added 🗄 area/interface This affects the public interface 🙆 yes/confirmed This is confirmed and ready to be worked on and removed 👀 no/external This makes more sense somewhere else labels Apr 14, 2020
@felixroos
Copy link

I am not sure if this is directly related, but parcel-plugin-mdx fails bundling mdx files that use fragments. Example use case:

## MDX Test

import { State } from "react-powerplug"

<State initial={{ count: 1 }}>
  {({ state, setState }) => (
    <>
      <p>{state.count}</p>
      <button onClick={() => setState({ count: state.count + 1 })}>
        increment
      </button>
    </>
  )}
</State>

Getting error:

react-dom.development.js:27795 Uncaught TypeError: Cannot read property 'mdxType' of null

I could also open a seperate issue..
Huge thanks for mdx as it is a very convenient way to write

@likerRr

This comment has been minimized.

@treyhuffine

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Nov 13, 2020

These three comments above are unrelated to the original issue. The original issue was something fixed by Babel, but as they’ve changed it in their v8, we still need support for @jsxFrag here, and we have an open pr for that: #1014.

For the new comments, this seems to be a different bug around fragments. I’d appreciate it if someone could debug some more and either post a new issue or maybe even raise a PR?

wooorm added a commit that referenced this issue Dec 19, 2020
This adds a `/* @jsxFrag mdx.Fragment */` next to the existing
`/* @jsx mdx */` pragma.
From MDX runtimes, this exports as `mdx.Fragment` either `React.Fragment`
or `Preact.Fragment`.

Vue 2 does not support fragments, but as JSX and hence MDX is already
specific to React or Vue, well: folks shouldn’t use fragments in MDX
files targeting Vue.

As we have fragments, we can also use that to pass children through
missing components: `<>{props.children}</>`.
This fixes runtimes where HTML is not available, such as React Native.
But, as Vue doesn’t like that, there’s a hidden flag to still use
the original behavior: `<div {...props} />`.
Still, there remains a difference in frameworks: Vue does not put
`children` in `props`, so `{...props}` has never passed children along
in Vue.

Closes GH-972.
Closes GH-990.
Closes GH-1014.
wooorm added a commit that referenced this issue Dec 20, 2020
This adds a `/* @jsxFrag mdx.Fragment */` next to the existing
`/* @jsx mdx */` pragma.
From MDX runtimes, this exports as `mdx.Fragment` either `React.Fragment`
or `Preact.Fragment`.

Vue 2 does not support fragments, but as JSX and hence MDX is already
specific to React or Vue, well: folks shouldn’t use fragments in MDX
files targeting Vue.

As we have fragments, we can also use that to pass children through
missing components: `<>{props.children}</>`.
This fixes runtimes where HTML is not available, such as React Native.
But, as Vue doesn’t like that, there’s a hidden flag to still use
the original behavior: `<div {...props} />`.
Still, there remains a difference in frameworks: Vue does not put
`children` in `props`, so `{...props}` has never passed children along
in Vue.

Closes GH-972.
Closes GH-990.
Closes GH-1014.
wooorm added a commit that referenced this issue Dec 20, 2020
This adds a `/* @jsxFrag mdx.Fragment */` next to the existing
`/* @jsx mdx */` pragma.
From MDX runtimes, this exports as `mdx.Fragment` either `React.Fragment`
or `Preact.Fragment`.

Vue 2 does not support fragments, but as JSX and hence MDX is already
specific to React or Vue, well: folks shouldn’t use fragments in MDX
files targeting Vue.

As we have fragments, we can also use that to pass children through
missing components: `<>{props.children}</>`.
This fixes runtimes where HTML is not available, such as React Native.
But, as Vue doesn’t like that, there’s a hidden flag to still use
the original behavior: `<div {...props} />`.
Still, there remains a difference in frameworks: Vue does not put
`children` in `props`, so `{...props}` has never passed children along
in Vue.

Closes GH-972.
Closes GH-990.
Closes GH-1014.
wooorm added a commit that referenced this issue Dec 20, 2020
This adds a `/* @jsxFrag mdx.Fragment */` next to the existing
`/* @jsx mdx */` pragma.
From MDX runtimes, this exports as `mdx.Fragment` either `React.Fragment`
or `Preact.Fragment`.

Vue 2 does not support fragments, but as JSX and hence MDX is already
specific to React or Vue, well: folks shouldn’t use fragments in MDX
files targeting Vue.

As we have fragments, we can also use that to pass children through
missing components: `<>{props.children}</>`.
This fixes runtimes where HTML is not available, such as React Native.
But, as Vue doesn’t like that, there’s a hidden flag to still use
the original behavior: `<div {...props} />`.
Still, there remains a difference in frameworks: Vue does not put
`children` in `props`, so `{...props}` has never passed children along
in Vue.

Closes GH-972.
Closes GH-990.
Closes GH-1014.
Closes GH-1394.

Reviewed-by: Christian Murphy <[email protected]>
Reviewed-by: John Otander <[email protected]>
@wooorm wooorm added 💪 phase/solved Post is done and removed good first issue 👋 This may be a great place to get started! help wanted 🙏 This could use your insight or help 🙆 yes/confirmed This is confirmed and ready to be worked on labels Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done
Development

Successfully merging a pull request may close this issue.

8 participants