-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
MDX is not compatible with @babel/plugin-transform-react-inline-elements #1327
Comments
@ivan-aksamentov could you expand on why you see this as something that should be fixed on the MDX side? |
Hi @ChristianMurphy,
It might be. I am not qualified to say on which side it should be fixed, because I don't know how either side works. I only wish, as a user of both, that they would work together. I submitted to MDX, and not Babel, because I've got an impression that the similar issue in styled-components was fixed on the component's side. Also I did not encounter any breakage with this plugin in years. My, rather naive, understanding is that MDX performs code generation at some point (and I might be wrong), and maybe the generated code does not play well with this particular optimization. I don't necessarily suggest that it's a serious defect in MDX, but rather that improving compatibility, if this is possible, would be nice.
I don't know what "a standard build" would be, but in my experience (in proprietary software mostly), this plugin is rather widespread. But here is also GitHub search: link. None of these 6000+ projects can use custom MDX components. And if they try, they will be having really hard times finding what prevents it from working. Took me a better part of the day to find the cause. This optimization is also something that is explicitly supported, albeit with caveats, by React: facebook/react#3228 Not pointing fingers here or anything. Long story short, I will be very glad if maintainers could look into the issue. I tried to do my best to investigate, and hopefully it will help. And if so happens that nothing can be done on MDX side, we can, of course, bring it up with babel folks too. |
The @babel/plugin-transform-react-inline-elements seems to be breaking MDX's ability to use custom components. See mdx-js/mdx#1327
Hi @ivan-aksamentov! Sorry you’re running into this! Your example repo is rather big, could you make the reproduction smaller? e.g., are all |
Hi @wooorm ,
I believe so, otherwise Next.js would not not recognize JSX or anything at all pretty much:
Okay, I removed whatever I found, without breaking things - styled-components, plugins for readable diffs, prettier and various files in the root. I don't think we can go any thinner without removing Next.js and writing a webpack config. I also updated the diffs in
Just to reiterate, these are 2 functions that are needed to produce readable diffs of the bundle. I thought that might be handy. I added them after the initial repro, for debugging. Now removed. But if you change your mind, and want to produce these readable diffs of the bundle again: git checkout 148ba81 and to see how styled-components are doing: git checkout eff63f8 |
Thanks @ivan-aksamentov! I cannot run your code, installing yarn and next globally, running
Could you either remove more things so as to only have the two projects in question, or specify more information on what you are using to reproduce to this issue? |
@wooorm Hm.. but it's there: No need to install |
Alright, so |
@wooorm Ah, yes. P.S. Please update me if you find something. Really curious how you would approach the problem. |
Thanks, I can indeed reproduce your problem in your repo. I don’t know next though and am personally not interested in going through all the magic they are doing. For me to debug more, I’d appreciate something with only the bare minimum dependencies: babel/plugin-transform-react-inline-elements and an mdx project. Maybe others do have that domain knowledge though? |
@wooorm You mean to remove Next.js and write a plain webpack config? I think I can do this, if that helps. But this will probably only add even more dependencies and boilerplate (which are hidden in Next.js now). Or without a bundler entirely? But then how to deal with React and JSX? What will we run as an executable? That babel plugin is specific for React components. |
I currently can’t rule out whether this is in next or any of the things they are doing. To get to the bottom of this, we need to figure out what MDX is doing wrong (if anything), and removing everything that is unrelated would help me. But maybe someone else has a the next domain knowledge to figure it out. React asks a similar question when you raise an issue there: https://github.com/facebook/react/blame/62efd9618b5027816cf7f8b54f5fc80b3d7af8ec/.github/ISSUE_TEMPLATE/bug_report.md#L23-L25. |
@wooorm Okay, so before the gurus of Next.js arrived, I sketched a plain webpack+babel version on branch I used this as a reference: To run:
Then open The result is pretty much the same as before. I also updated the diff in Is this new version helpful at all? |
Thanks! I have no clue why this is happening, maybe ask the babel folks? Btw, most of the diff you’re seeing is because the heading component uses an arrow, where import * as React from "react";
import * as ReactDOM from "react-dom";
import { MDXProvider } from "@mdx-js/react";
import Content from "./content.md";
function h1() {
return <span>!!!heading 1</span>;
}
function a() {
return <span>!!!link</span>;
}
export default function Index() {
return (
<MDXProvider components={{ h1, a }}>
<Content />
</MDXProvider>
);
}
ReactDOM.render(<Index />, document.getElementById("app")); Produces: var layoutProps = {};
var MDXLayout = "wrapper";
function MDXContent(_ref) {
var components = _ref.components,
props = content_objectWithoutProperties(_ref, ["components"]);
return createElement(MDXLayout, content_extends({}, layoutProps, props, {
components: components,
mdxType: "MDXLayout"
}), /*#__PURE__*/_jsx("h1", {}, void 0, "Heading 1"), /*#__PURE__*/_jsx("p", {}, void 0, createElement("a", content_extends({
parentName: "p"
}, {
"href": "http://example.com"
}), "link")));
}
...
function h1() {
return /*#__PURE__*/src_jsx("span", {}, void 0, "!!!heading 1");
}
function a() {
return /*#__PURE__*/src_jsx("span", {}, void 0, "!!!link");
}
function Index() {
return /*#__PURE__*/src_jsx(esm_MDXProvider, {
components: {
h1: h1,
a: a
}
}, void 0, /*#__PURE__*/src_jsx(MDXContent, {}));
}
react_dom["render"]( /*#__PURE__*/src_jsx(Index, {}), document.getElementById("app")); ...and I don’t know what |
Did you take their notes into consideration? https://babeljs.io/docs/en/babel-plugin-transform-react-inline-elements#note |
last idea: react is changing some of their “jsx runtime” stuff, we’re doing the classic type and setting the pragma inline, but maybe (some of) your code is using the new runtime? |
I’m assuming this can’t land, because the plugin you’re using is specific to React, whereas MDX currently hijacks |
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.
Subject of the issue
When combined with
@babel/plugin-transform-react-inline-elements
, MDX often not picking up the custom components provided throughMDXProvider
and renders the default components instead. This is probably due to heavy optimizations this babel plugin performs on react components. Removing the plugin from babel configuration fixes the issue.This plugin is only typically used in production and so the problem will not manifest during development or in tests. Therefore this breakage may slip unnoticed to production. In fact, I just shipped a broken version yesterday.
Wouldn't it be nice for MDX to support configurations with this babel plugin?
Your environment
OS: Ubuntu 20.04
Packages:
Env:
Steps to reproduce
I created a small (but not minimal) example demonstrating the issue here:
https://github.com/ivan-aksamentov/repro-mdx-inline-elements
It's based on Next.js and also contains styled-components (but none of this matter, see below).
The points of interest are:
src/pages/index.jsx
src/components.jsx
src/content.md
babel.config.js
In order to reproduce the bug, run the production version of this app with:
yarn install next build && next start
and navigate to
localhost:3000
. You will see something like this:Note how all of the components are styled with plain useragent stylesheet (meaning most of the custom components ARE NOT picked up). Inspect the HTML code for the "link" in dev tools and note that it DOES have
target="_blank" rel="noopener noreferrer"
attributes (meaning that the customLinkExternal
component IS picked up). Also note how>>>> LinkExternal <<<<<
is printed in build console (in terminal), but not>>>> H1 <<<<<
(these areconsole.log()
statements in the corresponding components).Go to
babel.config.js
and comment-out the line containing'@babel/plugin-transform-react-inline-elements'
.Cleanup, rebuild and restart the app.
rm -rf .build out .cache .next next build && next start
It should look like this now:
Note that all cusom components now work correctly: styled components' classnames are present, h1's text is replaced, backrgounds and margins are correctly styled, both console statements are printed in the build log, as expected. The
LinkExternal
component also still works as before. Reversing the change inbabel.config.js
reverts the fix.Styled components don't play a role here, I just wanted to show that they are working as well, because they are important for my usecase. With these components,
babel-plugin-styled-components
, and associated npm packages removed the issue still persists.Expected behaviour
It is expected that the custom components are picked up, whether the
@babel/plugin-transform-react-inline-elements
is used or notActual behaviour
Custom components are not picked up when
@babel/plugin-transform-react-inline-elements
is usedDiscussion
Let's generate a readable bunde code and see what is changing when adding/removing the plugin.
I added two Next.js plugins
withoutMinification()
andwithFriendlyChunkNames()
, which remove code minification and hashes from filenames in production build.I did the following experiment:
prepared output directories:
rm -rf compare/{good,bad}/** mkdir -p compare/{good,bad}
with babel plugin DISABLED, produced static build for "GOOD" version (static build has exactly the same issues as normal build, but it's easier to make sense of files it produces):
with babel plugin ENABLED, produced static build for "BAD" version:
compared the resulting directories with webstorm:
the only difference seems to be in
pages/index.js
generated a diff file for
pages/index.js
diff -u compare/{good,bad}/pages/index.js > compare/index.js.diff
one could also use
diff-so-fancy
to see the pretty diff in terminal:diff -u compare/{good,bad}/pages/index.js | diff-so-fancy
You can find the results in
compare/
, directory, inluding thecompare/index.js.diff
I was not able to make sense of the diff yet.
Unrelated to diff, but interestingly, the reason
LinkExternal
works seems to be the fact that it uses (renders)children
props. Addingchildren
toH1
component also fixes theh1
rendering. So props seems to be influencing the code optimizations in question. However, side effects, likeconsole.log()
don't seem to be preserved (notice how they are not printed during build) Sadly, this workaround will not work for styled components.Related issues in the community
styled-components
had a seemingly similar issue, and were able to solve it, whileemotion
given up on this: link1, link2.There is also a similarly useful plugin, @babel/plugin-transform-react-constant-elements. So far it does not seem to cause any breakage. However, it woth keeping an eye on it as well.
Possible workarounds
remove
@babel/plugin-transform-react-inline-elements
, paying extra runtime performance and bundle size cost.use
children
prop in custom components - does not work for many components, like styled-components, or components that are not meant to have children.Update:
Perhaps webstorm's diff is a bit more readable (left side is "good", right side is "bad"):
Looks like something is going on with this added function, as well as with
null
andvoid 0
arguments on call site. Still cannot tell what's wrong. For example,console.log()
calls are present in H1 on both sides, but are not working on the right side, while working on the left side.Update 2:
It may make more sense to examine the output of the normal
next build
in.next/static/chunks
(rather than of static export).The text was updated successfully, but these errors were encountered: