Remove JSX from output by default #1413
Conversation
wooorm
added
🦋 type/enhancement
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mdx/mdx/klodxnuc6 [Deployment for 31c6ed3 failed] |
| @@ -6,10 +6,6 @@ const minifyWhitespace = require('rehype-minify-whitespace') | |||
| const mdxAstToMdxHast = require('./mdx-ast-to-mdx-hast') | |||
| const mdxHastToJsx = require('./mdx-hast-to-jsx') | |||
|
|
|||
| const pragma = `/* @jsxRuntime classic */ | |||
| /* @jsx mdx */ | |||
| /* @jsxFrag mdx.Fragment */` | |||
There was a problem hiding this comment.
I also thought to put the comments in the estree instead
| }, | ||
| {loader: path.resolve(__dirname, '..'), options} | ||
| ] | ||
| use: [{loader: path.resolve(__dirname, '..'), options}] |
There was a problem hiding this comment.
a lot of the tests can be much simpler too
There was a problem hiding this comment.
generally looks good.
One caveat I will note, in the new setup mdx depends on estree-util-build-jsx, which in turn depends on astring (in the docs).
astring hasn't been actively maintained in the past year, and it only supports a subset of ESTree supported by acorn.
Meaning an example like:
<>{a?.b}</>will parse as expected in acorn (without babel), but will crash an generating a js file (due to optional chaining not being supported in astring).
|
@ChristianMurphy astring did land in the previous PR: #1399, where I described some considerations about it, essentially: I see your point. The nice thing about astring is that it’s easy to extend it (e.g., how I added JSX in core). Not sure if escodegen also supports that (as it doesn’t support JSX). |
|
Apologies I was unclear in my comment, I updated to reflect that the documentation and MDX treat
ESTree code gen is going to live under the unified umbrella? 😅 But I digress. |
|
Was this approved as a breaking change for v2? changing the compile target of MDX (and thus the expectation of all consumers, such as next/gatsby plugins as well as manual consumers) seems like it would have been discussed. |
|
@ChristopherBiscardi see #1041 under "Performance Considerations" and #1152 |
|
@ChristianMurphy I don't see anything in those issues discussing making a breaking change to accomplish this in v2. Perhaps you can link me to a comment directly? |
|
Previously, we required an extra build step to produce runnable code. This change makes the output of MDX immediately runnable. This drops the final requirement on Babel (Or Bublé). Dropping Babel leads to a size and performance win for the runtime (and for any use case that doesn’t otherwise require Babel, such as running in Node). Of course, if people want to use the latest JavaScript features, they can still use Babel, but it’s not *required*. Finally, if JSX is preferred (for example, Vue treats JSX radically different from other hyperscript interfaces and has its own JSX builders), `keepJsx` can be set to `true`. In short, the size breakdown for the runtime is: * `@mdx-js/runtime@1.6.22` (last stable tag): 356.4kb * `@mdx-js/runtime@2.0.0-next.8` (last next tag): 362.9kb * Previous commit (on an unmaintained Bublé fork): 165kb * This commit: 120kb (26% / 66% / 69% smaller) Core only adds ±1kb to its bundle size, because `estree-util-build-jsx` reuses dependencies that we already use, too. Related to GH-1041. Related to GH-1044. Related to GH-1152.
There was a problem hiding this comment.
If you want to say something, say it
To be clear, I meant exactly what I was asking in this thread and no more. We shouldn't be making arbitrary breaking changes in V2 without giving users the opportunity for a smooth upgrade.
You’re a maintainer here. A good time to discuss the aspects of this change was 6 months ago on the linked issue 🤷♂️
You're implying that this was spec'd out as a breaking change (in that we would be dropping support for the default behavior) to be included in v2, it was not. It being a breaking change is your choice and it is one I disagree with (as a maintainer, like you said).
In general, we should not be making non-critical breaking changes and dropping support for what worked before (like changing the default target format of MDX and removing the previous target) on a project that's getting 1.2MM downloads per week. End-users and integration owners should get time to upgrade incrementally and integration authors should not have to release "*-plugin-mdx-v2" packages or worry about which version of MDX they're running.
This was tagged and milestoned as v2 over the summer by @johno when he was still at Gatsby, I’m assuming his reasoning was not arbitrary, nor do I see it as such: performance and making it easier to use MDX are good reasons.
I’m unclear what about this is breaking?
It sounds like you’re particularly worried about |
JS is a subset of JSX, anything that ran before should still run, there's also a
There's a balance to this, at this scale Hyrum's law comes into play
Everything could be a breaking change. |
|
I want to jump in here and make it clear that I don't think this should be the default behavior for v2, but I love having this as an option that's configurable. To me, making this the default is sort of lumping in additional changes before a major drops which is poor strategy IMO. We want to make it compelling for folks to upgrade and this is primarily a hurdle. It's important to keep in mind that the majority of users are entering the ecosystem from frameworks like Next/Gatsby/Docusaurus/Storybook. Incremental paths to adoption (of v2) is what we should be prioritizing. This fundamentally breaks any downstream package/framework for little benefit. The way I see it is that integrations can migrate to this now JS output over JSX without having to do the work upfront.
This is true, but I don't think it's what we should do by default at this point. As an option the Gatsby integration can toggle the option for users in an opaque fashion. Ultimately this is an implementation detail. Also, IIRC MDX output still requires a Babel pass in Gatsby, so the primary gain in perf is already seen by the work for v2 internals because MDX no longer has to use Babel itself. I think it's important to also mention, MDX compilation has never been the bottleneck in Gatsby's integration.
Storybook, Docusaurus, Docz, MDX Deck, Toast, and any other of the growing list of folks that include MDX by default (IIRC Remix fits that as well).
I respectfully disagree, anyone doing any type of post-processing is now hit with a new AST that's way clunkier to deal with. JSX is more abstracted, JS presumes implementation of the function call.
Given the ease of the toggle, and it being less of an end user API and moreso for metaframeworks we should let them toggle it on when needed. I appreciate the discussion here but I think we should be optimizing for fewer API changes and getting v2 shipped. This would involve continuing to keep JSX output as the default. |
I’m OK with that!
I don’t think compiling JSX away is a hurdle. I can’t see what it breaks and I’m missing proof that it does: take the tests, 100% coverage before and after without changes. I think it’s a nice feature. Again: an option, defaulting to off is fine to me for now; so is strategically not bundling too much; but I also see value in adding cool features, faster performance, and smaller bundle sizes, for folks that wouldn’t otherwise upgrade for the actual breaking syntax changes.
I think it’s important to note that MDX is dictating exactly how JSX is compiled to JS: there is no wiggle room. There are three projects that I know of that turn JSX into JS: Bublé, which is deprecated; React; and the project I created for this PR. They all work the same, and they‘re all a no-op if there is no JSX. So: JSX -> JS calls here is not breaking anything. It used to be required to run Babel’s transform after MDX, it’s now done already and that Babel transform is a no-op.
This is a list of projects using MDX, I’m struggling to understand how they depend on JSX instead of JS in the output (more info above). Specifically, I see no code in Gatsby, Docz, or Docusaurus that depends on a certain flavour of output This is not to say that something might break: it might break someone’s build so we might as well put it in a major. But it shouldn’t, and I don’t see those project being hurt.
This is true! I have not seen that though, so in my opinion those folks could turn that toggle on or off to get what they desire when they upgrade. Or we can add an Again, I’m fine with delaying this feature as we’re already jampacked, but would still suggest to add it as its been promised and also because I think folks can appreciate a release more if it’s not just breaking changes. And: this PR becomes much less relevant due to GH-1425 |
The index.tsx page is now index.mdx. This required a change to the babel config as the JSX pragma in use caused issues with the mdx loader but this issue is being worked on. mdx-js/mdx#1413 We seem to now require React to be explicitly imported
|
landed! |
|
Progress in here? AMAZING! Thank you @wooorm !!! |
Previously, we required an extra build step to produce runnable code. This change makes the output of MDX immediately runnable.
This drops the final requirement on Babel (Or Bublé). Dropping Babel leads to a size and performance win for the runtime (and for any use case that doesn’t otherwise require Babel, such as running in Node). Of course, if people want to use the latest JavaScript features, they can still use Babel, but it’s not required.
Finally, if JSX is preferred (for example, Vue treats JSX radically different from other hyperscript interfaces and has its own JSX builders),
keepJsxcan be set totrue.In short, the size breakdown for the runtime is:
@mdx-js/runtime@1.6.22(last stable tag): 356.4kb@mdx-js/runtime@2.0.0-next.8(last next tag): 362.9kbCore only adds ±1kb to its bundle size, because
estree-util-build-jsxreuses dependencies that we already use, too.Related to GH-1041.
Related to GH-1044.
Related to GH-1152.