-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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: Fix static source handling for addon-docs #20147
Conversation
02d988c
to
9d074e6
Compare
@@ -0,0 +1,15 @@ | |||
import type { Plugin } from 'vite'; |
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.
Kept a plugin so vite-config stayed clean
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 really simplifies things nicely! The only part I'm not sure about is getting the settings from addon-docs. @shilman is this something that @storybook/csf-plugin
itself should be doing, maybe?
// @ts-expect-error - not sure what type to use here | ||
addons.find((a) => [a, a.name].includes('@storybook/addon-docs'))?.options ?? {}; | ||
|
||
return vite(docsOptions?.csfPluginOptions); |
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.
It feels to me like @storybook/csf-plugin
itself should take care of this...
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.
I don't think csf-plugin should know about the user's main.js and it makes sense for addon-docs to glue things together like this. Hopefully this all goes away in 7.x with the annotations server & simplifies things even further.
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.
Looks great! 💯
// @ts-expect-error - not sure what type to use here | ||
addons.find((a) => [a, a.name].includes('@storybook/addon-docs'))?.options ?? {}; | ||
|
||
return vite(docsOptions?.csfPluginOptions); |
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.
I don't think csf-plugin should know about the user's main.js and it makes sense for addon-docs to glue things together like this. Hopefully this all goes away in 7.x with the annotations server & simplifies things even further.
Issue: #19882 #19554
What I did
How to test
If your answer is yes to any of these, please make sure to include it in your PR.