Skip to content
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

feat: allow definition of stories in vue files #505

Merged
merged 10 commits into from
Jan 9, 2023

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Sep 29, 2022

I'm working on a vite plugin that transforms a vue sfc file to a standard Storybook CSF, so that one could specify stories in a format that is more convenient for vue users. This is inspired by histoire and will implement storybookjs/storybook#9768.

In this setting, stories are specified in xyz.stories.vue files, and then in a first step transformed to valid CSF js files. For this to work, the other vite plugins need to include or exclude these vue story files. This is done in this PR.

(As a follow-up, it would probably make sense to determine the files that the vite plugins operate on by looking at the stories config option in .storybook/main.js).

@IanVS
Copy link
Member

IanVS commented Sep 29, 2022

Hey, this is awesome news! You might also want to take a look at https://github.com/storybookjs/addon-svelte-csf, which is in a similar vein of allowing stories to be written in svelte-native format. You might want to take a look at storybookjs/addon-svelte-csf#69 and the associated PR, which adds support for index file generation and compatibility with the test-runner.

Have you considered creating a storybook addon which would add your vite plugin? In storybook 7.0, the vite builder will use the vite.config.js from the user, which means we won't be able to add an excludes like you've done here. If you'd like to hop into discord and chat about it, I'm happy to discuss ideas. Feel free to post in the #vite channel. discord.gg/storybook. I'd love to help make this a reality!

@tobiasdiez
Copy link
Contributor Author

Thanks for your encouraging words and for the pointer to the svelte addon. I'm not very deep into the storybook environment, so its good to see how other projects are doing this. I learned a few good tricks while reading the code.

The storybook addon is a good idea and would simplify the installation for the user. Do you have an example of an addon modifying vite-builder's vite config?

In storybook 7.0, the vite builder will use the vite.config.js from the user, which means we won't be able to add an excludes like you've done here.

My issue was that without this exclude the transformation added by my vite plugin would have been overwritten by the vite/vue plugin. As there doesn't seem a possibility to stop a transformation chain in vite (from a plugin) and my plugin needs to be quite early in the chain, it was easiest to simple exclude the stories file from the vite/vue plugin. I guess it would also be possible (although not very nice) to instruct users to add the exclude mechanism to their vite config in v7 (or maybe do this automatically via the addon).

(Thanks for the invitation to discord, but I'm not a big fan of that platform. Would prefer to continue the discussion here or in https://github.com/tobiasdiez/unplugin-storybook-vue, or via email.)

@IanVS
Copy link
Member

IanVS commented Sep 29, 2022

Do you have an example of an addon modifying vite-builder's vite config?

I don't think there are any addons which do this yet, but the frameworks do. See https://github.com/storybookjs/storybook/blob/next/code/frameworks/vue3-vite/src/preset.ts#L22 for instance.

Note that this will run before the user's viteFinal (if they have one in their .storybook/main.js).

and my plugin needs to be quite early in the chain

Could you unshift the plugin onto the list of plugins, rather than push, so it ends up at the top?

@tobiasdiez
Copy link
Contributor Author

Do you have an example of an addon modifying vite-builder's vite config?

I don't think there are any addons which do this yet, but the frameworks do. See https://github.com/storybookjs/storybook/blob/next/code/frameworks/vue3-vite/src/preset.ts#L22 for instance.

Note that this will run before the user's viteFinal (if they have one in their .storybook/main.js).

Thanks, this looks simple enough. Is this a v7 specific feature or would it work with v.6.5 as well?

and my plugin needs to be quite early in the chain

Could you unshift the plugin onto the list of plugins, rather than push, so it ends up at the top?

That's what I'm currently doing, and its working in the sense that its executed before the other plugins from builder-vite. However, I do need to exclude stories files from the vite/vue plugin, which runs later and would otherwise overwrite the changes.

https://github.com/tobiasdiez/unplugin-storybook-vue/blob/3a8fed21f315bee55d324be2f9917c425ccef473/examples/vite/.storybook/main.ts#L25

@tobiasdiez
Copy link
Contributor Author

Short update from my side: The basic things are now working. In particular, I've started to add an one-to-one correspondence between the "classical" CSF format and the native vue format for the stories that are in the documentation, see https://github.com/tobiasdiez/unplugin-storybook-vue/tree/main/examples/vite/src/writing-stories.

Feedback on the current state is much appreciated. Where I'm currently stuck is the handling of args in a way that feels native to vue. I've added an initial proposal that is up for discussion at tobiasdiez/storybook-vue-addon#13. What do you think?

@IanVS
Copy link
Member

IanVS commented Nov 9, 2022

@Dschungelabenteuer, @shilman I wonder if maybe you have some thoughts on this exciting work to add native vue stories. See @tobiasdiez's note just above, and the links he provided.

@tobiasdiez
Copy link
Contributor Author

I've managed to find a workaround for the compatibility with the vue plugin, so the exclude hack is no longer necessary. The remaining changes of this PR are still needed to run or not run the plugin on the new stories.vue file.

It would be nice if this PR could be merged, so that I can release an early alpha for the vue-storybook addon that people can try out and give feedback on. 🚀

@Dschungelabenteuer
Copy link
Member

Sorry I didn't have time to take a deep dive into the plugin itself yet! However, there are a few thoughts and questions which cross my mind while taking a quick look at @tobiasdiez's work:

  • From a Vite+Vue user's perspective, this looks promising and this could be a very interesting alternative way of writing stories. Some of my colleagues are not quite familiar with Storybook and – despite the CSF format being pretty simple to use and get used to – it might prove more natural for them to write plain Vue SFCs.

  • From the perspective of a potential user of this plugin, this raises two main questions for you @tobiasdiez. Sorry if you've already answered them before, I could not find this piece of information without going through your code:

    • Are you intending to make this work for both Vue 2 and 3? Vue 3 user here, but I know that for various reasons, its adoption is quite slower than expected among the Vue ecosystem, and that there is probably still a bunch of Storybook users relying on Vue 2. IIRC, while Storybook's Vite builder initially did not officially supported Vue 2, @IanVS worked on both Vue 2.6 and 2.7 support recently.
    • Since you went for an unified plugin, would the webpack5 builder be supported? My guess is, if it required some changes on Vite builder's side, it would also require some changes on the webpack one's?
  • From a contributor's perspective, the actual changes of this PR made me wonder what would maintainers' take be here. As a maintainer, I would probably want to keep the codebase as much framework-agnostic as possible, and some of the changes here introduce a bit of framework specificity. Maybe some work could be done here to provide users with a way of customizing patterns and extensions used to match stories.

Anyway, this looks cool and I'll surely give it a try soon :)

@tobiasdiez
Copy link
Contributor Author

Thanks for the feedback! Concerning your points:

  • Yes, the CSF format is easy enough to understand and write, but I'm really missing IDE support (Volar) when writing more complex stories/templates.
  • I mostly focused on Vue 3 currently. But the code currently doesn't use anything specific to that version and the only dependency on vue is via the vue sfc compiler that is used to compile the template to a render function. So if the demand arises one could make the vue compiler a peer dependency and reuse whatever the user has installed.
  • Webpack support is indeed planned, but I haven't yet looked at this. Since I'm not that familiar with the Storybook + Webpack combo, I would appreciate PRs in this direction. Shouldn't be too hard.
  • I agree, it would be nice to refactor the whole filter mechanism to match the user-defined stories (and probably use vite's filter method https://github.com/rollup/plugins/tree/master/packages/pluginutils#createfilter).

@tobiasdiez
Copy link
Contributor Author

@IanVS Can you please review this PR? Thanks!

@IanVS
Copy link
Member

IanVS commented Nov 23, 2022

Hi, yes I will indeed find some time to review this, hopefully soon.

@IanVS
Copy link
Member

IanVS commented Dec 7, 2022

I haven't forgotten about this, just been busy lately! Just wanted to let you know it's on my list of things to get to.

Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this took me so long to get to. I think this is fine to try out. I made a small suggestion to improve the regexes, and I'm a little nervous about the source loader running on .vue files, but I'm willing to give this a shot and let you test things out further with your addon. Mind updating the regexes, and I'll get this merged and released? As long as @joshwooding doesn't have any concerns.

packages/builder-vite/plugins/vue-docgen.ts Outdated Show resolved Hide resolved
packages/builder-vite/source-loader-plugin.ts Outdated Show resolved Hide resolved
packages/builder-vite/inject-export-order-plugin.ts Outdated Show resolved Hide resolved
packages/builder-vite/code-generator-plugin.ts Outdated Show resolved Hide resolved
packages/builder-vite/vite-config.ts Outdated Show resolved Hide resolved
tobiasdiez and others added 5 commits January 8, 2023 19:14
Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
Co-authored-by: Ian VanSchooten <ian.vanschooten@gmail.com>
@tobiasdiez
Copy link
Contributor Author

Thanks for the review. I've now committed your suggested improvements to the regexes.

Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool let's get this out there and we can iterate as needed. Thanks!

@IanVS IanVS merged commit f0d9d7e into storybookjs:main Jan 9, 2023
@tobiasdiez tobiasdiez deleted the vue-stories branch January 9, 2023 03:51
@tobiasdiez
Copy link
Contributor Author

Thanks you very much! I've now released a first early-preview of the addin at https://www.npmjs.com/package/storybook-vue-addon. Please feel free to play around with it if you have the time.

@bodograumann
Copy link
Contributor

This looks quite interesting. Have you thought about using the new native format as an alternative to writing stories and documentation in mdx format, @tobiasdiez ?

@tobiasdiez
Copy link
Contributor Author

These experiments are still at a very early phase and I've not thought about the integration with mdx/documentation. How is this handled by the native svelte addon?
Maybe we could use the MDC format of nuxt/content to embed vue components and thus stories into markdown?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants