-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Vite/Addon-docs: Fix customization of MDX plugins #20116
Conversation
ab3f5e9
to
52ab68f
Compare
skipCsf: true, | ||
...mdxPluginOptions, |
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.
@shilman How do you think we should merge these configs?
This setup always respects skipCsf but merges everything else
c88d18d
to
930a4d2
Compare
253f95a
to
7825e7e
Compare
I see this is still a draft, what is left to do before it is ready for review? |
Was hoping to get some guidance on my earlier questions |
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.
@joshwooding This is the current Webpack configuration in 7.0
https://github.com/storybookjs/storybook/blob/next/code/addons/docs/src/preset.ts#L46-L53
I'm wondering how this change squares with that. It's also possible that we made an unintentional breaking change there & we need to restore some level of configuration there that we deleted with the last round of changes. WDYT?
7825e7e
to
504049f
Compare
@shilman let's discuss tomorrow? |
7d14b62
to
3b38180
Compare
@@ -15,10 +15,8 @@ const isStorybookMdx = (id: string) => id.endsWith('stories.mdx') || id.endsWith | |||
export async function mdxPlugin(options: Options): Promise<Plugin> { | |||
const include = /\.mdx?$/; | |||
const filter = createFilter(include); | |||
const addons = await options.presets.apply<StorybookConfig['addons']>('addons', []); |
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.
@shilman addons wasn't defined when doing this. When I debugged the code it seems like I can get the options directly?
3b38180
to
21dc26f
Compare
I've tested this locally with:
and it works |
21dc26f
to
a9f388e
Compare
const docsOptions = | ||
// @ts-expect-error - not sure what type to use here | ||
addons.find((a) => [a, a.name].includes('@storybook/addon-docs'))?.options ?? {}; | ||
const { mdxPluginOptions, jsxOptions } = await options.presets.apply<Record<string, any>>( |
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.
This behavior is really strange. Neither @ndelangen or I know where options
is coming from. Somehow this works. Merging and I hope that we can get rid of this code in 8.0 when we get rid of defining stories in MDX.
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.
Yeah, I was quite surprised too. I only found it when debugging through the code.
@joshwooding you're a hero! ❤️ |
Blocked by storybookjs/mdx2-csf#28Replaces #20096
Closes #20116
Issue:
What I did
How to test
If your answer is yes to any of these, please make sure to include it in your PR.