-
-
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
Always treat _createMdxContent as a JSX component #2445
base: main
Are you sure you want to change the base?
Conversation
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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. |
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 |
That’s a very different issue. I don’t see how that’s related.
I’m not sure what you mean by this. Perhaps I’m missing something you’re seeing? |
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.
A hook can exist in the provider. |
Ah, I thought you didn’t want this transform in case for example 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 |
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.
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 |
@remcohaszing Coming back to this, where are we standing? 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 |
Initial checklist
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 theMDXContent
component conditionally. This breaks React’s rule of hooks.Closes #2444