Closed ivan-aksamentov closed 3 years ago
@ivan-aksamentov could you expand on why you see this as something that should be fixed on the MDX side?
From the description of the issue, the MDX build works in a standard build, but @babel/plugin-transform-react-inline-elements
produces invalid output in some cases.
This seems like something which could alternatively be fixed by @babel/plugin-transform-react-inline-elements
itself not producing invalid output?
Hi @ChristianMurphy,
why you see this as something that should be fixed on the MDX side?
This seems like something which could alternatively be fixed by @babel/plugin-transform-react-inline-elementsitself not producing invalid output?
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.
MDX build works in a standard build
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: https://github.com/facebook/react/issues/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.
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 next/babel
things needed? Can you cut something from the next config? Ideally, a good reproduction would only use the strictly required packages: mdx-js/react with @babel/plugin-transform-react-inline-elements.
Hi @wooorm ,
are all next/babel things needed?
I believe so, otherwise Next.js would not not recognize JSX or anything at all pretty much: https://nextjs.org/docs/advanced-features/customizing-babel-config
could you make the reproduction smaller?
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 compare/
directory.
Can you cut something from the next config?
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 yarn
, and then:
$ next build && next start
The module 'react' was not found. Next.js requires that you include it in 'dependencies' of your 'package.json'. To add it, run 'npm install react'
The module 'react-dom' was not found. Next.js requires that you include it in 'dependencies' of your 'package.json'. To add it, run 'npm install react-dom'
node:internal/modules/cjs/loader:903
throw err;
^
Error: Cannot find module 'react'
Require stack:
- /Users/tilde/.nvm/versions/node/v15.1.0/lib/node_modules/next/dist/bin/next
at Function.Module._resolveFilename (node:internal/modules/cjs/loader:900:15)
at Function.Module._load (node:internal/modules/cjs/loader:745:27)
at Module.require (node:internal/modules/cjs/loader:972:19)
at require (node:internal/modules/cjs/helpers:88:18)
at Object.<anonymous> (/Users/tilde/.nvm/versions/node/v15.1.0/lib/node_modules/next/dist/bin/next:26:13)
at Module._compile (node:internal/modules/cjs/loader:1083:30)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1112:10)
at Module.load (node:internal/modules/cjs/loader:948:32)
at Function.Module._load (node:internal/modules/cjs/loader:789:14)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:72:12) {
code: 'MODULE_NOT_FOUND',
requireStack: [
'/Users/tilde/.nvm/versions/node/v15.1.0/lib/node_modules/next/dist/bin/next'
]
}
$ node -v; npm -v; yarn -v; next -v
v15.1.0
6.14.8
1.22.10
The module 'react' was not found. Next.js requires that you include it in 'dependencies' of your 'package.json'. To add it, run 'npm install react'
The module 'react-dom' was not found. Next.js requires that you include it in 'dependencies' of your 'package.json'. To add it, run 'npm install react-dom'
Next.js v10.0.1
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 next
globally. Just clone, cd <project_dir>
and yarn install
. It will install Next.js from package.json
.
Alright, so $ ./node_modules/.bin/next build && ./node_modules/.bin/next start
for me then
@wooorm Ah, yes. ./node_modules/.bin
should be in the $PATH
. Or yarn next
would also work (which searches in ./node_modules/.bin
for binaries). I should have added this to package scripts.
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 webpack
:
https://github.com/ivan-aksamentov/repro-mdx-inline-elements/tree/webpack
I used this as a reference: https://mdxjs.com/getting-started/webpack https://babeljs.io/setup#installation and then patched things up until it worked.
To run:
git pull && git checkout webpack
yarn install && yarn webpack
Then open dist/index.html
in a browser (e.g. drag and drop).
The result is pretty much the same as before. I also updated the diff in compare/
directory.
Now we know that Next.js is not at fault.
Is this new version helpful at all? Can MDX be ran without a bundler and babel loader to trim things down even more? How would I approach that?
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 a
is just a function, index.jsx
like so:
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 babel-plugin-transform-react-inline-elements
is basing the difference in the a
and h1
on? 🤷♂️
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 createElement
calls (related: https://github.com/mdx-js/mdx/issues/1385), and so it’s impossible to “preval” virtual nodes at build time.
Sorry, I don‘t see it happening.
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
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:
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.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:
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
one could also use
diff-so-fancy
to see the pretty diff in terminal: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).