Always treat `_createMdxContent` as a JSX component

[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)
• [x] If applicable, I’ve added docs and tests

Description of changes

If the user provides a wrapper component, _createMdxContent was treated as a JSX component. Otherwise it was invoked as a function. Because _createMdxContent uses a hook, this hooks becomes part of the MDXContent component conditionally. This breaks React’s rule of hooks.

Closes #2444

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mdx	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 25, 2024 0:58am

[wooorm]

Using a function improves performance, that's why it's a function instead of a component. It also allows for diffing; the recommendation is to call the MDXContent yourself if it works even. The provide function is not always called. And, does not necessarily contain a hook. Should most users loose performance because of this? Should non-react users, or people who use different or no providers, get a slower experience?

[remcohaszing]

I get your point, though I have my doubts about the significance of the performance impact.

An alternative partial solution could be to let the React and Preact providers default the wrapper comonent to Fragment. This fixes the problem if wrapper is toggled via a provider. It doesn’t fix the problem if the user then explicitly passes a wrapper component via props and toggles that between undefined and a defined value.

[wooorm]

I have my doubts about the significance of the performance impact.

You didn’t before? https://github.com/mdx-js/mdx/issues/2029, https://github.com/mdx-js/mdx/pull/2062.

---

The compiler knows when it injects provideComponents. I am surprised you don’t want to go about it with that info?

[remcohaszing]

I have my doubts about the significance of the performance impact.

You didn’t before? #2029, #2062.

That’s a very different issue. I don’t see how that’s related.

---

The compiler knows when it injects provideComponents. I am surprised you don’t want to go about it with that info?

I’m not sure what you mean by this. Perhaps I’m missing something you’re seeing?

[wooorm]

Huh, I thought it was added in a PR somewhere. I can’t find the trace quickly in xdm other than https://github.com/wooorm/xdm/commit/f0fde3b5c1f6d2de8c0b40aba63f027b6d7edcfd. Anyway, see https://github.com/mdx-js/mdx/issues/1655 for more on speed.

I’m not sure what you mean by this. Perhaps I’m missing something you’re seeing?

A hook can exist in the provider. We can support a hook in the provider, but not decrease performance for people that do not use providers, by switching functionality based on whether the compiler is configured with a provider. You can make the changes in this PR optional.

[remcohaszing]

Ah, I thought you didn’t want this transform in case for example @mdx-js/vue or a third party provider is used as providerImportSource.

Another alternative I’m thinking of is something along these lines:

  function _createMdxContent(props) {
-   const _components = {
-     ..._provideComponents(),
-     ...props.components,
-     p: 'p',
-   }
+   const _components = props.components
  }

  export default function MDXContent(props = {}) {
-   const {wrapper: MDXLayout} = {
+   const components = {
      ..._provideComponents(),
      ...props.components,
+     p: 'p',
    }
+   const MDXLayout = components.wrapper
    return MDXLayout
-     ? <MDXLayout {...props}>
-         <_createMdxContent {...props} />
-       </MDXLayout>
-     : _createMdxContent(props)
+     ? <MDXLayout {...props} components={components}>
+         <_createMdxContent {...props} components={components} />
+       </MDXLayout>
+     : _createMdxContent({...props, components: components})
  }

The benefit is that _provideComponents() is only called once. The downside with this is that the components prop constructed does not have a stable identity.

[wooorm]

in case for example @mdx-js/vue

Well, that would be quite nice yeah. That we only have a slow path for providers that might need it. And inline it for those that don’t. But I don’t know how. And generally I’d recommend against providers. So it’s not the common case.

_provideComponents() is only called once

I believe that shouldn’t happen. Component resolution needs to be in the tree at the right place. Otherwise it would break context based APIs: components defined in MDX files (exported from them) also use the provider for “missing” components.

Someone can inject more components in MDXLayout too. That’s why this whole _createMdxContent mess exists...

[wooorm]

@remcohaszing Coming back to this, where are we standing? Reading through the code, I guess, the primary thing that is missing is an actual test that used to fail or warn, and works now. And, can we make this optional: Only do <_createMdxContent> where there are providers being passed.

Reading through the thread, what would be nice, is some benchmark: I wrote this code, this way as a function, because it is faster.

Looking at the OP again, perhaps we can also solve this by generating a _components = _provideComponents() in _createMdxComponents(), even when it is unused?