storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.59k stars 9.31k forks source link

Preact: Missing addon-docs support #19739

Open yannbf opened 2 years ago

yannbf commented 2 years ago

Preact does not seem to have proper addon-docs support, which results in:

Seems like react-docgen kind of supports Preact, so maybe it's not that complicated to add support?

KrofDrakula commented 1 year ago

@yannbf Preact uses TS generics and parameter types to define the args consumed by each component. Would it be sensible to just build a TS scanner that would extract the relevant types and convert them to a format that Storybook can consume?

I've adapted a similar mechanism in Preview.js from the reference React implementation where the TS parser and AST are used to extract all of the relevant types, and can even include JSDoc, which is the preferred way Preact components are documented from what I've seen in the wild.

Would this approach make sense, or would you prefer adapting react-docgen for this?

IanVS commented 1 year ago

@shilman this might be a good question for you, regarding how to proceed with docgen support for preact.

@KrofDrakula are you offering to create a preact-docgen package which would do what you suggested, scanning TS/AST to create the same kind of output as react-docgen? That seems like it would be awesome to me, and could benefit other projects other than Storybook as well.

KrofDrakula commented 1 year ago

@IanVS That is correct. I think it wouldn't be that hard to create tooling that would support any kind of React-like component definition, and it might allow Storybook to leverage more than what react-docgen offers, and give every other framework the tools to quickly implement type and docs extraction with a few minor tweaks. This would include Inferno, Mithril, Solid, etc. I would start with Preact though before making it generic.

Of course, the caveat is that it wouldn't support sources like Marko and frameworks that use custom templates and conventions like Svelte and Vue would have to include extra processing, but it would be better than starting from scratch, and the expected output types would be exposed to other authors looking to extend support for other libs.

shilman commented 1 year ago

@KrofDrakula This sounds like a huge undertaking. I bet it won't be hard to get something working, but getting it working well across the hundreds of corner cases will be a lot of maintenance.

@kroeder has done something similar for Angular here https://github.com/storybookjs/webpack-angular-types-plugin

I'm not sure about the current status of that project or how it relates to your proposal, but I'd like to understand that better before commenting.

I've heard a hand wave proposal for a generic TS extractor a few times and would love to see how that could support both angular AND preact, for example.

kroeder commented 1 year ago

Just took a super quick look at this, thanks for the ping @shilman

We had to lower the priority for the plugin and planned proceeding this in Q1/23

And yes, we also talked about further steps to make it a plugin system where typescript is supported by default and on top it is possible to add extraction plugins for framework specifics

But that said, we haven't even tested this for angular in a broader audience. though, besides of a few bugs, it works pretty well in our companies framework and is actively used in production for months now

KrofDrakula commented 1 year ago

@IanVS @shilman @kroeder Interesting, that's exactly the kind of approach I was thinking of; using adapter functions for each framework, using TS AST as the intermediate data model, and providing many transform functions to output to tools like @storybook/addon-docs.

I've already started playing with the idea in a PoC project on my own to explore the feasibility of the idea. I'll take a peek at the Angular implementation and see if there is a way to leverage existing work to speed things up.

In terms of how to proceed with this issue — would you rather I try to get react-docgen to add support, or should I experiment with these two?

shilman commented 1 year ago

@KrofDrakula Since you already have some experience in this area, and interest, and the payoff could be large, I'd love for you to spend some time experimenting with your approach. I remain skeptical, but would love to be proven wrong!

KrofDrakula commented 1 year ago

@shilman Happily!

KrofDrakula commented 1 year ago

So, just to clarify:

For a given component, is the intention to return ArgsTableProps as the description? Or would it be better to return Record<string,ArgType>?

shilman commented 1 year ago

The latter, ArgTypes, is the right return type. ArgsTableProps is more of an implementation detail currently. @KrofDrakula

KrofDrakula commented 1 year ago

@shilman @IanVS So, finally have some good news to share. :)

I've sketched out a couple of complex types in a separate repo to demonstrate TS' ability to correctly infer types. Here are the examples I've successfully been able to extract types from:

Right now, I haven't built the translation layer to ArgTypes yet, but that will be the next step. That will include things like resolving JSDoc and related types.

Unfortunately, the compiler API types are not really documented (other than reading related projects and the TypeScript source), so progress has been slow and steady. At least this way, we can add as many complications per framework as can be found in the wild and easily test the extractions/translations.

IanVS commented 1 year ago

@kroeder That's awesome news! Maybe check out https://ts-morph.com (but maybe that's already one of the related projects you mentioned). I've worked with that in the past, and it was a nicer than trying to deal with the compiler directly.

kroeder commented 1 year ago

@kroeder That's awesome news! Maybe check out https://ts-morph.com (but maybe that's already one of the related projects you mentioned). I've worked with that in the past, and it was a nicer than trying to deal with the compiler directly.

ts-morph is the fundament of the mentioned plugin :)

KrofDrakula commented 1 year ago

@IanVS That's exactly what I used. :)

https://github.com/KrofDrakula/prop-docs/blob/21e4cbc1e288f0c7b575ba3d7ba1d176aac570bf/packages/preact/src/extract_components.ts

KrofDrakula commented 1 year ago

@shilman @IanVS After a bit of fiddling and bug reports, I've got the initial PoC implementation for the Storybook adapter working:

https://github.com/KrofDrakula/prop-docs/blob/dffd24b939810c6407a6ddf84fce3940ff0d2152/packages/storybook/src/convert_type.test.ts#L17-L77

The approach will enable pairing any framework adapter with any output format like Storybook's ArgTypes, so as long as libraries don't dramatically change their underlying API, this should be pretty stable.

IanVS commented 1 year ago

That's great news! What do you think are the next steps?

KrofDrakula commented 1 year ago

Obviously fleshing out the different cases that describe types, so far I've only gotten the bare minimum and would like to support more specific and granular configs as per Controls, and I haven't added support for parsing args from CSF 3. Once I'm happy that the library has a robust repertoire of type extractors, the next step would be to start a branch and start implementing this in the core.

I'll probably want a second pair of eyes to assist in the integration, or I could help guide the implementer since the final product will simply take a ts-morph Project (this isn't set in stone yet) as input and a source file name, and get back a Map<string, ArgTypes> of component and story names. Preferrably as a single function requiring no setup beyond supplying the instantiated Project (which is just the TypeScript Program wrapped in a façade).

The other thing is that I'm keeping a separation between the TypeScript language version and ts-morph I use to extract the types; that way, a peerDependency to this package will ensure that you're not locked to a particular version of the language, hence why I am still looking into ways of supplying the initial required setup. It may end up just taking the tsconfig path and include/exclude to configure the peer TypeScript compiler, depending on how integration feedback goes.

Thoughts?

KrofDrakula commented 1 year ago

@shilman @ndelangen @IanVS @kasperpeulen So, good news and bad news.

First, some bad news — it looks like resolving type aliases required to resolve types like ComponentProps<Component<{ name: string }>> to resolve to { name: string } is something left as an exercise to the compiler API user. I have been able to resolve some type aliases based on a couple of assumptions, but this breaks down when using more complex compositions.

The good news is that the PreviewJS project publishes a couple of packages including @previewjs/type-analyzer that does the math for us. At this point, I haven't determined how much work it would be to bind the plugin to that package instead of mine, but it looks a lot more flexible and correct than what I could build on my own (and reinvent the wheel in the process). The upside is it also has support for Vue 2 & 3, SolidJS and Svelte as well, so extending its analyzer support would also be possible once we get Preact up and running.

Considering that we already have things like #21192 to fill in the gaps in the meantime, I was thinking that it might be worth pivoting towards that library instead, and dropping my own package in favor of this approach.

On the other hand, the code I have now is minimal and covers most of the common cases for simple definitions. I could publish what I have now as a stopgap for Preact and migrate later.

ccsenter commented 10 months ago

@KrofDrakula @shilman In SB v7.6.4 the fact that the Preact framework and renderer still does not have an out-of-box support for docs annotations is something of a bit of concern for us (Oracle JET team). This has a long history, I first reported this back in 2021 (see https://github.com/storybookjs/storybook/issues/14876) and I was hoping that this effort by Klemen would finally get some traction. I see with some sadness that the last comment was almost a year ago. We worked around this issue until now based on @shilman comments/suggestion from 2021 when he suggested to create the same (or close enough) configuration/setup for the preact renderer as the react one. At that time the feature matrix indeed showed that there was no full addon-docs support yet for preact. We've been doing this (workaround) for a while, forked SB, added the supporting classes/modules to the preact renderer (to match that of the react renderer), built the module and we checked-in into our repo as a SB patch that we installed whenever our monorepo was installed. However it would we extremely wise from our end not to keep doing this pattern. The current feature matrix right now shows full docs support for Preact: https://storybook.js.org/docs/configure/frameworks-feature-support#community-frameworks but I think that's wrong. See how https://github.com/storybookjs/storybook/blob/next/code/renderers/preact/src/entry-preview-docs.ts does not expose the extractArgTypes property which is a must for ArgTypes to work as expected. What can be done to finally have full ArgTypes support for the Preact framework? Originally I was thinking to just submit a PR with the code changes we have today in our fork (again, it's almost a 1-1 copy of the React renderer relevant code to the Preact renderer) but reading through this open issue, I see that there might be a more generic solution brewing in @KrofDrakula mind :-) What do you guys suggest? Thanks for reading through!

shilman commented 10 months ago

@ccsenter All preact maintenance is community-driven so we don't do much active development other than reviewing PRs and broad maintenance work to keep it up to date when we rearchitect Storybook's core. If you were to submit a PR with your changes I'd be happy to review and try to figure out what it would take to get it shipped.

A few things to note for context:

  1. As of Storybook 8, we are shipping react-docgen as the default for @storybook/react, because it is dramatically faster than react-docgen-typescript and they have overcome the most critical limitations in their TypeScript support. So it would be nice if your solution worked with that too. But beggars can't be choosers so I'll take what I can get. 😄
  2. At some point, I would like to do a generic ts-morph-based solution, inspired by @KrofDrakula 's work, and make it work for all the different frameworks. But it's not on the roadmap and we have a lot of projects we're prioritizing first, so I wouldn't hold my breath for it.
ccsenter commented 9 months ago

@shilman Thanks for the speedy feedback, I'm tackling the react-docgen issue as I prepare the PR.

ccsenter commented 9 months ago

I submitted the PR: https://github.com/storybookjs/storybook/pull/25630 This was my first one so...please bare with me :-)

ccsenter commented 9 months ago

Hm...I just realized that the code changes I have in my feature branch were based off of SB 7.6.4...I presume I should re-create these changes based on a synced next branch in my fork.

ccsenter commented 9 months ago

@shilman I closed the PR (see my previous comment about the wrong version of the base). I re-created my changes on a feature branch that was created off of next however creating a sandbox is broken. I'm not able to create the default react-vite/default-ts because failes with:

Downloading sandbox template (before-storybook)...
🚨 Failed to download sandbox template: TypeError: fetch failed
🚨 Failed to create sandbox

Should I still create the PR or wait until the sandbox issue is fixed?