Always treat _createMdxContent as a JSX component

[remcohaszing]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)
• 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]

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? #2029, #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 wooorm/xdm@f0fde3b. Anyway, see #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?

[wooorm]

@remcohaszing Ping! :)

[wooorm]

I would appreciate an answer to my above questions. Including the one on whether “_components = _provideComponents() in _createMdxComponents(), even when it is unused?” works. Perhaps all this code is not needed. We can only know this if there are actual tests and benchmarks that were broken before and are improved now.

[remcohaszing]

I would appreciate an answer to my above questions.

Sorry for the delay. I’m really busy with other things right now!

---

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.

The test involves React state updates. I suppose the best way to test it is React Testing Library.

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.

I understand that calling _createMdxComponents(props) could be faster than _jsx(_createMdxComponents, { ...props }). However, I really doubt its significance. We’re talking about avoiding the creation of a single JSX element. It might even be slower, because it could trigger rerenders of the JSX inside _createMdxComponents that wouldn’t happen when using JSX. A benchmark could be interesting, but without a benchmark, the current implementation seems like a premature optimization. I can test this using React devtools and a simple React app when I have time.

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

Has calling useContext() (via _provideComponents()) a lower performance impact than calling _jsx().

---

All of my above answers are about React. The exact impact could of course vary per JSX implementation.

[wooorm]

We’re talking about avoiding the creation of a single JSX element.

It’s the top element. The problem is the diffing. See #1655. This is why the generated code is this way.

Has calling useContext() (via _provideComponents()) a lower performance impact than calling _jsx().

That is unknown. People can choose what they do in _provideComponents.
The current state is optimized to not call if not needed.
React chooses to break when this is optimized.
React chooses if context is slow or fast.

Plus: OP’s problem is about “Delete all content of test.mdx”. That is what produces this problem with React. How often does that happen? Most likely, an empty MDX document resulting in an unused call to useContext() will be fast enough. Whereas my hunch is that changing the top to be regenerated for every character for every document, will be significant.