Fixes #990 #1014
Fixes #990 #1014
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/mdx/mdx/206fbqv5r |
|
thanks for your patience, @adammockor! I think this will break things, yes, because |
|
Would you be interested in continuing work on this? E.g., adding some tests to make sure it works, and also for Vue? For my earlier comment, adding both |
|
I am not sure I get "automatic" and "classic" runtime stuffs yet, but it seems that the "automatic" runtime expects you not to set |
|
Edit: Looking at the comment from Andarist below, it looks like the Yeah, if both plugins / presets support the automatic runtime, then you do not need to use the pragma comments:
|
|
Maybe if you need to continue using the comments on a file-by-file basis (instead of configuring it for the whole project like in my example), the https://babeljs.io/docs/en/babel-plugin-transform-react-jsx#customizing-the-automatic-runtime-import |
| @@ -124,7 +124,7 @@ async function compile(mdx, options = {}) { | |||
|
|
|||
| const {contents} = await compiler.process(fileOpts) | |||
|
|
|||
| return `/* @jsx mdx */ | |||
| return `/* @jsxFrag React.Fragment */ | |||
There was a problem hiding this comment.
Edit: This is wrong, looking at the comment from Andarist below. It looks like the mdx packages will need to be changed.
Maybe something like this?
| return `/* @jsxFrag React.Fragment */ | |
| return `/** @jsxImportSource mdx */ |
There was a problem hiding this comment.
113 return `/* @jsxRuntime classic */
114 /* @jsxFrag React.Fragment */
115 /* @jsx mdx */
116 ${contents}`
117 }
129 return `/* @jsxRuntime classic */
130 /* @jsxImportSource mdx */
131 /* @jsx mdx */
132 ${contents}`
133 }
it's also not working in ubuntu 20.4
|
@sgmheyhey these three lines are incompatible: /* @jsxRuntime classic */
/* @jsxImportSource mdx */
/* @jsx mdx */As far as I understand, you want EITHER:
/* @jsxImportSource mdx */
/* @jsxRuntime classic */
/* @jsx mdx */...but then again, I am not an expert here. @Andarist has been super helpful in the past elsewhere (with the integration of Emotion and Next.js). |
Yes, they are incompatible. @jsxImportSource mdxThis means that there is an
From what I understand none of this is true - especially given that @jsxRuntime classic + @jsx mdxThis is most likely the most correct answer here for now. We'll probably be lifting the |
This comment has been minimized.
This comment has been minimized.
|
@Andarist Ah ok thanks! Alright, so it sounds like according to your comment above, the
Then what needs to happen for compatibility with the new
I'm guessing these changes are probably also similar to the changes in emotion-js/emotion#1970 I've edited my comments above. |
I’m working on trying to see if ditching JSX completely is feasible, which would compile MDX to |
|
OK, so to solve this, I think we should add, to these two lines Lines 9 to 10 in 3783554 This code: Then, in For |
|
Note - I will hopefully file a PR to Babel this week to somewhat make |
|
@Andarist Oh, that sounds interesting! I’d love to read that PR if you’ve posted it. |
|
Sure, we'll try to remember to cc you there - just need to find time for a rebase and adding few tests. |
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.
I suppose any usage of the classic runtime is only a temporary workaround? Since there's downsides to it and the new JSX transform looks like the way to go, going forward? |
|
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.
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.
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 <christian.murphy.42@gmail.com> Reviewed-by: John Otander <johnotander@gmail.com>
|
Not sure if it's related to this one but I'm trying to use Theme UI with MDX and I'm getting: with the following |
|
I don’t think the |
|
The problem is very similar to not being able to use 2 classic pragmas at once. I believe that MDX could use the automatic runtime just fine (it's just a different way of declaring the overwrite) but it still would not play OK with other custom automatic runtimes - as there is no built-in way to compose them. |
|
Oh, so you mean that there is currently no way to specify multiple // NOT WORKING
{
"presets": [
[
"@babel/preset-react",
{
"runtime": "automatic",
// DO NOT USE, DOES NOT WORK
"importSource": ["theme-ui", "mdx"]
}
]
]
}Interesting drawback - is there anything being worked on to add this capability to compose runtimes? Or to do something like this, would a new wrapper library need to be created (eg. |
|
@Andarist “The problem is very similar to not being able to use 2 classic pragmas at once.” The same can be said for two automatic runtimes too though, right? Or generally: two runtimes? @karlhorky “is there anything being worked on to add this capability to compose runtimes?” How would that work if both react and preact are used? React/preact/vue/other hyperscripts are actual renderers (“ Emotion really needs to add props to all elements, but I’m personally of the opinion that MDX does not need a runtime/renderer/ |
|
Hmm that's an interesting drawback of this design... I'm wondering how it's going affect tooling. I agree it feels like Theme UI shouldn't need pragma for their I'm going to take a look at the package you mentioned but the problem there are so many things that start to depend on each other and debugging it is a nightmare. |
@wooorm Yeah, good question. I'm not as deep into this architecture as I should be to seriously propose anything, but FWIW I guess my idea was based on functional composition, where you can put two functions that have a similar input and output API and string them together (eg. So I suppose I was probably thinking about something similar to what you were mentioning with
|
Probably this. And it seems that such a package already exists - as @wooorm has found out.
Yes, that's exactly what I have meant - sorry if the intention was not clear. |
This PR moves most of the runtime to the compile time. This issue has nothing to do with `@mdx-js/runtime`. It’s about `@mdx-js/mdx` being compile time, and moving most work there, from the “runtimes” `@mdx-js/react`, `@mdx-js/preact`, `@mdx-js/vue`. Most of the runtime is undocumented features that allow amazing things, but those are in my opinion *too magical*, more powerful than needed, complex to reason about, and again: undocumented. These features are added by overwriting an actual renderer (such as react, preact, or vue). Doing so makes it hard to combine MDX with for example Emotion or theme-ui, to opt into a new JSX transform when React introduces one, to support other hyperscripts, or to add features such as members (`<Foo.Bar />`). Removing these runtime features does what MDX says in the readme: “**🔥 Blazingly blazing fast: MDX has no runtime […]**” This does remove the ability to overwrite *anything* at runtime. This brings back the project to what is documented: users can still overwrite markdown things (e.g., blockquotes) to become components and pass components in at runtime without importing them. And it does still allow undocumented parent-child combos (`blockquote.p`). * Remove runtime renderers (`createElement`s hijacking) from `@mdx-js/react`, `@mdx-js/preact`, `@mdx-js/vue` * Add `jsxRuntime` option to switch to the modern automatic JSX runtime * Add `jsxImportSource` option to switch to a modern non-React JSX runtime * Add `pragma` option to define a classic JSX pragma * Add `pragmaFrag` option to define a classic JSX fragment * Add `mdxProviderImportSource` option to load an optional runtime provider * Add tests for automatic React JSX runtime * Add tests for `@mdx-js/mdx` combined with `emotion` * Add support and test members as “tag names” of elements * Add support and test qualified names (namespaces) as “tag names” of elements * Add tests for parent-child combos * Add tests to assert explicit (inline) components precede over provided/given components * Add tests for `mdxFragment: false` (runtime renderers w/o fragment support) * Fix and test double quotes in attribute values This PR removes the runtime renderers and related things such as the `mdxType` and `parentName` props while keeping the `MDXProvider` in tact. This improves runtime performance, because all that runs at runtime is plain vanilla React/preact/vue code. This reduces the surface of the MDX API while being identical to what is documented and hence to user expectations (except perhaps to some power users). This also makes it easier to support other renderers without having to maintain projects like `@mdx-js/react`, `@mdx-js/preact`, `@mdx-js/vue`: anything that can be used as a JSX pragma (including the [automatic runtime](https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html)) is now supported. A related benefit is that it’s easier to integrate with [emotion](https://github.com/emotion-js/emotion/blob/master/packages/react/src/jsx.js#L7) (including through `theme-ui`) and similar projects which also overwrite the renderer: as it’s not possible to have two runtimes, they were hard to combine; because with this PR MDX is no longer a renderer, there’s no conflict anymore. This is done by the compile time (`@mdx-js/mdx`) knowing about an (**optional**) runtime for an `MDXProvider` (such as `@mdx-js/react`, `@mdx-js/preact`). Importantly, it’s not required for other hyperscript interfaces to have a provider: `MDXContent` exported from a compiled MDX file *also* accepts components (it already did), and Vue comes with component passing out of the box. In short, the runtime looked like this: ```js function mdx(thing, props, ...children) { const overwrites = getOverwritesSomeWay() return React.createElement(overwrites[props.mdxType] || thing, props, ...children) } ``` And we had a compile time, which added that `mdxType` prop. So: ```mdx <Youtube /> ``` Became: ```js const Youtube = () => throw new Error('Youtube is not loaded!') <Youtube mdxType="Youtube" /> ``` Which in plain JS looks like: ```js const Youtube = () => throw new Error('Youtube is not loaded!') React.createElement(Youtube, {mdxType: 'Youtube'}) ``` Instead, this now compiles to: ```js const {Youtube} = Object.assign({Youtube: () => throw new Error('Youtube is not loaded!')}, getOverwritesSomeWay()) React.createElement(Youtube) ``` The previous example shows what is sometimes called a “shortcode”: a way to inject components as identifiers into the MDX file, which was introduced in [MDX 1](https://mdxjs.com/blog/shortcodes) A different use case for the runtime was overwriting “defaults”. This is documented on the website as the “[Table of components](https://mdxjs.com/table-of-components)”. This MDX: ```mdx Hello, *world*! ``` Became: ```js <p mdxType="p">Hello, <em mdxType="em">world</em>!</p> ``` This now compiles to: ```js const overwrites = Object.assign({p: 'p', em: 'em'}, getOverwritesSomeWay()) <overwrites.p>Hello, <overwrites.em>world</overwrites.em>!</overwrites.p> ``` This MDX: ```mdx export const Video = () => <Vimeo /> <Video /> ``` Used like so: ```jsx <MDXProvider components={{Video: () => <Youtube />}}> <Content /> </MDXProvider> ``` Would result in a `Youtube` component being rendered. It no longer does. I see the previous behavior as a bug and hence this as a fix. A subset of the above point is that: ```mdx export default props => <main {...props} /> x ``` Used like so: ```jsx <MDXProvider components={{wrapper: props => <article {...props} />}}> <Content /> </MDXProvider> ``` Would result in an `article` instead of the explicit `main`. It no longer does. I see the previous behavior as a bug and hence this as a fix. (#821) ```mdx <h2>World</h2> ``` Used like so: ```jsx <MDXProvider components={{h2: () => <SomethingElse />}}> <Content /> </MDXProvider> ``` Would result in a `SomethingElse` for both. This PR **does not** change that. But it could more easily be changed if we want to, because at compile time we know whether something was a tag or not. An undocumented feature of the current MDX runtime renderer is that it’s possible to overwrite anything: ```mdx <span /> ``` Used like so: ```jsx <MDXProvider components={{span: props => <b>{props.children}</b>}}> <Content /> </MDXProvider> ``` Would overwrite to become bold, even though it’s not documented anywhere. This PR changes that: only allowed markdown “tag names” can be changed (`p`, `li`, ...). **This list could be expanded.** Another undocumented feature is that parent–child combos can be overwritten. A `li` in an `ol` can be treated differently from one in an `ul` by passing `'ol.li': () => <SomethingElse />`. This PR no longer lets users “nest” arbitrary parent–child combos except for `ol.li`, `ul.li`, and `blockquote.p`. **This list could be expanded.** It was not possible to use members (`<foo.bar />`, `<Foo.bar.baz />`, <#953>) and supporting it previously would be complex. This PR adds support for them. Previously, `mdxType` and `parentName` attributes were added to all elements. And a `components` prop was accepted on **all** elements to change the provider. These are no longer passed and no longer accepted. Lastly, `components`, `props` were in scope for all JSX tags defined in the “markdown” section (not the import/exports) of each document. This adds identifiers to the scope prefixed with double underscores: `__provideComponents`, `__components`, and `__props`. A single 1mb MDX file, about 20k lines and 135k words (basically 3 books). Heavy on the “markdown”, few tags, no import/exports. 322kb gzipped. * v1: 2895.122856 * 2.0.0-next.8: 3187.4684129999996 * main: 4058.917152000001 * this pr: 4066.642403 * v1: raw: 1.5mb, gzip: 348kb * 2.0.0-next.8: raw: 1.4mb, gzip: 347kb * main: raw: 1.3mb, gzip: 342kb * this pr: raw: 1.8mb, gzip: 353kb * this pr, automatic runtime: raw: 1.7mb, gzip: 355kb * v1: 321.761208 * 2.0.0-next.8: 321.79749599999997 * main: 162.412757 * this pr: 107.28038599999996 * this pr, automatic runtime: 123.73588899999999 This PR is much faster on giant markdown-esque documents during runtime. The win over the current `main` branch is 34%, the win over the last beta and v1 is 66%. For output size, the raw value increases with this PR, which is because the output is now `/*#__PURE__*/React.createElement(__components.span…)` or `/*#__PURE__*/_jsx(__components.span…)`, instead of `mdx("span", {mdxType: "span"…})`. The change is more repetition, as can be seen by the roughly same gzip sizes. That the build time of `main` and this PR is slower than v1 and the last beta does surprise me a lot. I benchmarked earlier with 1000 small simple MDX files, totalling 1mb, [where the results were the inverse](#1399 (comment)). So it looks like we have a problem with giant files. Still, this PR has no effect on build time performance, because the results are the same as currently on `main`. This PR makes MDX faster, adds support for the modern automatic JSX runtime, and makes it easier to combine with Emotion and similar projects. --- Some of what this PR does has been discussed over the years: Related-to: GH-166. Related-to: GH-197. Related-to: GH-466 (very similar). Related-to: GH-714. Related-to: GH-938. Related-to: GH-1327. This PR solves some of the items outlined in these issues: Related-to: GH-1152. Related-to: #1014 (comment). This PR solves: Closes GH-591. Closes GH-638. Closes GH-785. Closes GH-953. Closes GH-1084. Closes GH-1385.
Replace /* @jsx mdx / with / @jsxFrag React.Fragment */.
This fixes problem for me (custom cra + mdx). Could it break something?