-
Notifications
You must be signed in to change notification settings - Fork 9
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
Remove JSX from compiler's page output #20
Conversation
This change is safe if we don't provide a way to change the jsx runtime. |
It works for me, good job, I had this exact issue a day before you submitted this.
We don't have a way for users to change the JSX runtime in docs - which I think also means that we don't support anything lower than React |
src/mdx2.test.ts
Outdated
@@ -100,7 +100,7 @@ describe('mdx2', () => { | |||
componentMeta.parameters = componentMeta.parameters || {}; | |||
componentMeta.parameters.docs = { | |||
...(componentMeta.parameters.docs || {}), | |||
page: () => <MDXContent />, | |||
page: () => _jsx(MDXContent, {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @shilman that maybe we could just do this instead. I tried it locally and it worked.
page: () => _jsx(MDXContent, {}), | |
page: MDXContent, |
src/sb-mdx-plugin.ts
Outdated
@@ -318,7 +318,7 @@ export const wrapperJs = ` | |||
componentMeta.parameters = componentMeta.parameters || {}; | |||
componentMeta.parameters.docs = { | |||
...(componentMeta.parameters.docs || {}), | |||
page: () => <MDXContent />, | |||
page: () => _jsx(MDXContent, {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
page: () => _jsx(MDXContent, {}), | |
page: MDXContent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IanVS I simplified this PR a bit based on the discussion with @JReinhold
This change won't be enough, since this compiler also generates JSX for stories declared in MDX:
https://github.com/storybookjs/mdx2-csf/blob/next/src/mdx2.test.ts#L132-L139
This feature of Storybook MDX is getting deprecated in 7.0, after which this package will no longer be necessary. We can talk about how we want to deal with that in the Vite builder. For example, we could continue to support the old behavior in the Webpack builder, but drop support in the Vite builder as a breaking change.
In this short term, I'm going to merge this PR and update next
in the monorepo as well. It shouldn't hurt anything and is an improvement over what's there currently.
Issue: N/A
Returning JSX syntax from the mdx transform is problematic for the vite builder. Previously, we added the
@vitejs/plugin-react
plugin to all non-react projects, so that we could use it to process the JSX being returned by this mdx compiler. The@mdxjs/mdx
compiler does not use JSX syntax, but rather uses the jsx runtime, by adding this to the top of the transformed code:What Changed
Instead of returning JSX syntax, I used the
MDXContent
directly as the component.With this change, I was able to remove the react vite plugin from non-react frameworks, and still render the introduction mdx and docs pages in the example docs. There may be other cases I haven't considered, though, like
.stories.mdx
files (are those still a thing?).So, I'm not positive if this is a safe change, or if more is needed as well, but I wanted to at least start a discussion about it and see what happens in CI.
How to test
I tested by making this change in the
node_modules
of a sandbox, and then starting up storybook.Change Type
maintenance
documentation
patch
minor
major
📦 Published PR as canary version:
0.0.4-canary.20.5648a54.0
✨ Test out this PR locally via:
npm install @storybook/mdx2-csf@0.0.4-canary.20.5648a54.0 # or yarn add @storybook/mdx2-csf@0.0.4-canary.20.5648a54.0
Version
Published prerelease version:
v0.1.0-next.2
Changelog
🚀 Enhancement
AddContext
#14 (@tmeasday)🐛 Bug Fix
Authors: 4