Generated JSX should not define component on rerender

[remcohaszing]

Initial checklist

• [X] I read the support docs
• [X] I read the contributing guide
• [X] I agree to follow the code of conduct
• [X] I searched issues and couldn’t find anything (or linked relevant results below)

Affected packages and versions

^2.0.0-rc.2 (#1810)

Link to runnable example

No response

Steps to reproduce

Run the following script:

import { compile } from '@mdx-js/mdx';

const { value } = await compile('hello', { jsx:true })

console.log(value)

The jsx option is there for readability of the output. The issue applies if it’s either true or false.

Expected behavior

The output doesn’t define a component inside a component. This is a React anti-pattern, because it causes unnecessary rerenders

The following Google search yields some articles that explain why this is a bad idea in React: https://www.google.com/search?q=react+create+component+inside+component. Example article: https://dev.to/borasvm/react-create-component-inside-a-component-456b. The same principles apply to Preact.

Earlier the output was ok:

/*@jsxRuntime automatic @jsxImportSource react*/
function MDXContent(props = {}) {
  const _components = Object.assign({
    p: "p"
  }, props.components), {wrapper: MDXLayout} = _components;
  const _content = <><_components.p>{"hello"}</_components.p></>;
  return MDXLayout ? <MDXLayout {...props}>{_content}</MDXLayout> : _content;
}
export default MDXContent;

Alternative output that’s also ok for React/Preact, although I recall it’s troublesome for some frameworks:

/*@jsxRuntime automatic @jsxImportSource react*/
import { Fragment as _Fragment } from 'react/jsx-runtime.js'
function MDXContent(props = {}) {
  const _components = Object.assign({
    p: "p"
  }, props.components), {wrapper: MDXLayout = _Fragment} = _components;
  return <MDXLayout><_components.p>{"hello"}</_components.p></MDXLayout>;
}
export default MDXContent;

Actual behavior

The output does define a component inside a component, which causes unnecessary rerenders.

/*@jsxRuntime automatic @jsxImportSource react*/
function MDXContent(props = {}) {
  const {wrapper: MDXLayout} = props.components || ({});
  return MDXLayout ? <MDXLayout {...props}><_createMdxContent /></MDXLayout> : _createMdxContent();
  function _createMdxContent() {
    const _components = Object.assign({
      p: "p"
    }, props.components);
    return <_components.p>{"hello"}</_components.p>;
  }
}
export default MDXContent;

Runtime

Node v16

Package manager

npm v8

OS

Linux

Build and bundle tools

Other (please specify in steps to reproduce)

[wooorm]

Unfortunately it’s required for context-based components passing inside MDXLayout to work. Before, components could not be set in the context of an MDXLayout. After, it can. If you don’t pass an MDXLayout, the function is called immediately, not as a component, so it does not affect users that don’t use layouts.

[remcohaszing]

I see the issue you’re describing.

I think the following would solve both this issue and the issue you described:

/*@jsxRuntime automatic @jsxImportSource react*/

function _createMdxContent(props) {
  const _components = Object.assign({
    p: "p"
  }, props.components);
  return <_components.p>{"hello"}</_components.p>;
}

function MDXContent(props = {}) {
  const {wrapper: MDXLayout} = props.components || ({});
  return MDXLayout ? <MDXLayout {...props}><_createMdxContent {...props} /></MDXLayout> : _createMdxContent(props);
  }
}
export default MDXContent;

[wooorm]

Perhaps! It looks good but I'm not 100% it'll work

[0phoff]

I think this inner component also blocks libraries that want to traverse and modify the react element tree (MDX deck, mdxp). When I print the generated MDXContent component in the console, it shows no children in it's props.

Since I would love to update MDXP to MDX2, I am willing to look into this issue and submit a PR. I imagine I would need to look into the recma plugin of @mdx-js/mdx. Do you have any pointers to get me started? I have some experience with remark/rehype but never looked at recma.

[wooorm]

I don’t believe traversing the react tree is the way to go, generally. We have ASTs and they’re much better.

The approach proposed above looks good. It waits for someone to work on it, indeed, so if that’s you, :+1:. It would go in core, probably around here:

https://github.com/mdx-js/mdx/blob/63fd208d53f13e5b2451c9f9bc1b7fd136d22c71/packages/mdx/lib/plugin/recma-document.js#L486

If you have experience with ASTs, then you should be able to figure out how the JS AST works, looking at ASTExplorer and the existing code. Let me know if you have questions

[0phoff]

I don’t believe traversing the react tree is the way to go, generally. We have ASTs and they’re much better.

I completely agree and was looking for a way to perform this as a rehype/recma plugin, but couldn't figure out how to do the following:

Users of my library can mark components as being a "slide layout component". If such a component is used as the sole component of a slide (between 2 <hr>), then I consider that to be a slide. If there are multiple components between the <hr> tags or the component is not defined as a layout, then I wrap the content in a default slide layout.

The way I mark a component as a slide layout is by adding a property MDXPType on the function and thus I think I can only perform this check at runtime. The only way I can think of to do this with unified at build time, is to either pass a list of slide layouts to the plugin or have a rule that slide layout components should always end in "SlideLayout". I don't really like either of these options and thus implemented my parsing at runtime in the client 😅

[wooorm]

This sounds like a whole, unrelated, problem. Perhaps you can open a discussion and share much more about what you want to do.

I don’t understand why MDXPType is added instead of the new MDX AST nodes. You can walk the AST, “split” on thematic breaks, and check if there’s only one thing in them, in which case you wrap content into a JSX element.

[shepmaster]

I think this inner component also blocks libraries that want to traverse and modify the react element tree (MDX deck, mdxp).

I had a similar problem, also with a slide deck implementation. I added a comment elsewhere describing what I did.