Storybook should use dist/ Rollup bundle, not build all over again

[dgreene1]

Current Behavior

If you want enhance the build (like adding fonts for instance), you currently have to do that in two places:

• .storybook/main.js where you would use Webpack's 'file-loader'
• tsdx.config.js where you would use '@rollup/plugin-url'

Desired Behavior

I would ideally not have to modify the Storybook build to pull in the fonts at all. By the very nature of telling rollup to include the fonts, Webpack would get them because it would be including the compiled output that lives in dist.

Suggested Solution

Who does this impact? Who is this for?

I'm not sure.

Describe alternatives you've considered

Additional context

[agilgur5]

A few things:

• This might be required for react-docgen-typescript-loader to work, but I'm not sure, it might be able to use the generated type declarations too
• The docs actually say Storybook also uses dist, but that's incorrect, only the example does.
• I'm not sure specifically if @rollup/plugin-url has a runtime component or how it works under-the-hood; you might still need to have file-loader. Been meaning to add an integration test for it for a bit to be able to test usage/issues with it.
  • I do think this is potentially problematic on the consistency side of things more broadly. E.g. you have Babel plugins but they're not used in the Storybook config.

I'll assign this to @kylemh because I'm not totally sure the intention behind compiling etc a second time around is vs. doing the same as the example / what the docs say.

[agilgur5]

Ah, welp looks like I can only assign people who have commented or collaborators. And I can't add @kylemh as a collaborator, only Jared can do that 😕
Moving this to an org would help with that because you can use teams (also can limit push access to master that way), but that's similarly only something Jared can do....

[kylemh]

I think this is a pretty low priority for me. I simply don’t mind it running again 🤷‍♂️

[dgreene1]

@kylemh please let me clarify why this is an extremely important goal. It’s not simply a matter of running twice

So imagine you configure your Storybook webpack file to properly in-line your SVGs but the rollup build wasn’t. Then you build your library (via rollup) and discover that your SVGs are missing only when a real user files a bug in production.

[kylemh]

So then the problem is you’re separating the configs beyond how they exist by default. Out of the box they compat fine 🤷‍♂️

Whatever you do to one config the equivalent should happen in the other (in case you won’t be resolving this issue with a PR)

I’m not saying this isn’t an issue. It's obviously better to build once and serve anywhere. This is just not a problem I’m encountering personally. I’ve got a few other issues in this repo I care a lot more about resolving.

[dgreene1]

So then the problem is you’re separating the configs beyond how they exist by default. Out of the box they compat fine 🤷‍♂️

I’m not separating the configs— they already were. In the template, there is a Storybook build (via webpack configured in .storybook/main.js) and a build for the library (via Rollup and configured in tsdx.config.json). As far as I am aware, there is no sharing between the build process besides the fact that the webpack watcher watches the folder that rollup writes to.

However, if webpack was set up to not read the library code from ./src but instead to read it from ./dist then Storybook would be loading the real library as if it were a true consumer. And that’s the goal that is currently not being achieved.

[kylemh]

It’s not so simple. As @agilgur5 mentioned there are some addons as part of the TS preset which won’t work if we simply target the existing bundle. Storybook has its own bundling process which we’d need to account for and translate to TSDX

[agilgur5]

@kylemh I pinged you because I wasn't sure of the intentionality of using src instead of dist when the docs and example say/do otherwise and I'm not sure if react-docgen-typescript-loader is able to read type declarations.
The change is just a one-liner to an import, so that's not difficult, I just don't know what other implications there are to that because my only experience with Storybook is that of supporting TSDX.

@dgreene1 I should've mentioned this at the beginning, but these are just templates, you can change your stories's import to just go from ../ instead of ../src like the example is set-up. Per Node module resolution, that makes it read from package.json (and Webpack will probably take the module ESM build).
If you try that, let us know here what the caveats of that are!

[kylemh]

Right. I already target dist in some cases when using TSDX + Storybook, but not all the time. My old hold up is exactly the concern you mentioned regarding react-docgen-typescript-loader.

[dgreene1]

I confirmed that react-docgen-typescript-loader appears to have trouble loading the prop table when it reads from dist. In addition to my own findings, it appears that there is an issue already out there requesting assistance on that: strothj/react-docgen-typescript-loader#97

For me, the value of seeing my widgets exactly as what is going to be exported is worth it for me, so I'm personally sticking with importing via dist instead of src.

However, I believe that Storybook 6.0 (with it's new and improved doc pages) is going to be including a zero-config typescript setup. So once that is published and this related issue (storybookjs/storybook#10738) is resolved, I would be happy to migrate the Storybook template in tsdx to utilize doc-pages instead of react-docgen-typescript-loader since doc-pages is an in-house storybook product (unlike react-docgen-typescript-loader)

[kylemh]

💯 there are a few issues touching on Storybook in this repo. I've honestly been waiting for v6 to release before I change anything here.

[kylemh]

Apologies for the delays @agilgur5 @shilman - got laid off and had to deal with a job hunt. Luckily, my new workplace wants me to get a component library going! I'm hoping to resolve all the issues assigned to me and then be a consumer again.

I'll be using Storybook v6 alpha as the release is scheduled for July 31st.

[kylemh]

@jaredpalmer since you moved this to the Formium org, I wanted to suggest that @agilgur5 be added as a member of the team so he can assign Storybook preset things to me. I'd gladly join the org too if you feel like I've done enough, but no worries if not.

[jaredpalmer]

Yeah makes sense. Sorry for the name changes. To avoid confusion with the open source library, we’re renaming what was formerly Formik Cloud to Formium.

[kylemh]

I'm just happy it's in an org now!

[kylemh]

Oh, he already seems to be a member and I was already assigned to a few issues. So, I just need to be assigned to this one.

[kylemh]

@dgreene1 regarding your efforts here: #702 (comment)

@shilman has a v6 alpha up and running: storybookjs/storybook#10738 (comment)

It looks like Storybook is using react-docgen-typescript-plugin now. Any way to quickly identify if it can generate tables properly from types?

[dgreene1]

There’s no quick way other than migrating fully to v6, which I wasn’t planning on doing till it’s fully released (not sure what the term is for when it’s no longer in alpha).

Someone else might need to try if you need an answer sooner. Otherwise I’ll be replying to this thread once v6 of storybook is fully released.

[kylemh]

No worries. I'll give it a whirl.

[shilman]

Still prerelease (RC now) and targeting final release for end of July earl August. There's still some loose ends, but the TypeScript side of things has held up pretty well so far during the RC and feels light years better than the hot mess it replaces. If anybody here can kick the tires, I'd love to make sure it plays nicely with TSDX before we do the final release! 🙏

[agilgur5]

@kylemh to clarify, I'm in fact not a "Member" of the org, I'm a "Collaborator" of the repo. "Collaborators" have a very limited set of permissions, basically write access and some community moderation things. Adding "Members" to "Teams" also allows one to add more fine-grained permissions, for instance, issue triage but not write-access, write-access but not admin. Branch protection also isn't set up (and I don't have permissions to set it), so write access means ability to change main branch as well (whereas PR-only is best practice).

[kylemh]

It's released, @dgreene1 !

I doubt this will be possible given the compilation process involved around react-docgen-typescript.

I spoke with @shilman about this (thanks for the reminder @agilgur5), and he suggested there may be a way to do upstream changes to react-docgen-typescript-plugin such that typedefs are enough to automatically generate docs. There's definitely still space for this issue, as it sounds like the Storybook team wants to give people the ability to leverage Storybook with prebuilt packages; however, I doubt this issue will be resolved for some time.

[dgreene1]

FYI @yhy-vertex this is the issue that's blocking our types from coming in.

[NogaMan]

Concerning the issue with broken docs when importing from dist

I solve it by wrapping components imported from dist folder with a HOC and reexporting them in src/index.tsx
I import the components for stories from src/index.tsx now instead of dist and storybook docs work properly ("no inputs found for this component" error went away)

Below is a simple HOC that does nothing but drilling props to a passed component and reexporting example (src/index.tsx):

import React from 'react';
import {
  DashedRectangle as DashedRectangleBase,
  ColorExamples as ColorExamplesBase,
} from '@scooter/core'; // this is pointing to a lerna package's dist folder, 

function withDocsEnabled<P>(Component: React.FC<P>) {
  return (props: React.PropsWithChildren<P>) => <Component {...props} />;
}

export const DashedRectangle = withDocsEnabled(DashedRectangleBase);
export const ColorExamples = withDocsEnabled(ColorExamplesBase);