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

Create / use react-docgen vite plugin to show docsPage info #299

Merged
merged 7 commits into from
Apr 3, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Apr 1, 2022

Fixes #103
Closes #190
Fixes #2

This takes a slightly different approach from #190. Instead of using a babel transform, this creates a vite plugin, similar to the vue-docgen plugin, but based on the babel-plugin-react-docgen. This gives us the chance to rely directly on react-docgen, without needing another package in between, and it avoids being accidentally overwritten if users replace our @vitejs/plugin-react with their own config.

This new vite plugin is not configurable like the babel plugin is, but I'm not certain those configurations are necessary in the first place. If we hear from users that it's something they need, we can find a way to make them customizable, but I prefer to avoid extra options if possible.

I used one of the most recent git commits to react-docgen, since their conversion to typescript happened after their last alpha release, and it was really useful having the types. Hopefully another alpha will be released soon, so the build won't need to happen during install time (a definite drawback, but hopefully fine temporarily. react-docgen is being actively developed after a long break. \o/. Monitor reactjs/react-docgen#502 for updates).
Edit: had to fall back to the alpha due to yarnpkg/berry#2578 breaking CI.

This version does not create a global list of all the docs, like the babel plugin does (DOC_GEN_COLLECTION_NAME). I looked through the storybook source, and it doesn't seem like anything still relies on it, so I think it's just a legacy detail for now that we can probably avoid supporting.

I've updated the react example to demonstrate the use of the plugin, so feel free to check this out there.

@IanVS IanVS requested review from eirslett and joshwooding April 1, 2022 05:44
@IanVS IanVS changed the title Create react-docgen vite plugin Create / use react-docgen vite plugin to show docsPage info Apr 1, 2022
@IanVS
Copy link
Member Author

IanVS commented Apr 1, 2022

Doh, looks like yarn in windows isn't happy with the git reference. Might have to wait for an alpha after all...

yarnpkg/berry#2578

I'll try using the current alpha for now.

@IanVS
Copy link
Member Author

IanVS commented Apr 1, 2022

This doesn't seem to fix #2, although our typescript example is working just fine with @joshwooding/vite-plugin-react-docgen-typescript.

Edit: was just missing a .storybook/preview.js file.

@IanVS IanVS force-pushed the react-docgen branch 2 times, most recently from 73aa788 to 85037af Compare April 1, 2022 21:02
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Generally LGTM. If we were going to use this long-term I would suggest that this gets its own package with tests. But I think this is temporary until we create an annotations server inside storybook. I tested it out and the results line up with the webpack version, so I think this is good to go.

name: 'react-docgen',
enforce: 'pre',
async transform(src: string, id: string) {
if (/\.(mjs|tsx|jsx)$/.test(id)) {
Copy link
Member

Choose a reason for hiding this comment

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

what about .js?

Copy link
Member Author

Choose a reason for hiding this comment

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

When using vite, it's required to use .jsx or .tsx for JSX syntax. But, I guess technically it's possible to create a react component using react.createElement(), so it doesn't hurt to check non-jsx files too. I'll change it.

@sebastienbarre
Copy link

I'd be happy to help test things out once it's in a testable shape, if you guys point me to the version I would need to switch to (currently at 6.4.20). Thanks.

@IanVS IanVS merged commit 1e9cbc5 into main Apr 3, 2022
@IanVS IanVS deleted the react-docgen branch April 3, 2022 23:53
@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

@sebastienbarre if you'd like to try this out, you can npm install --save-dev @storybook/builder-vite@^0.1.24. Thanks!

@sebastienbarre
Copy link

sebastienbarre commented Apr 4, 2022

Thanks @IanVS

Great work. This seems to be working the same way now as it was before I switched from the default Storybook builder (webpack) to Vite-builder.

Now unfortunately the way it was working before... wasn't doing the trick at all for me. I was hoping for a small miracle here, and I truly appreciate the effort nonetheless. My gut feeling is that react-docgen can't solve my issue at the moment (maybe in 6.0?).

To be more specific. The vast majority of my React components look like this, for better or worse (to simplify):

const shapes = {
  ROUNDED: 'rounded',
  CIRCULAR: 'circular',
  RECTANGULAR: 'rectangular',
};

const sizes = {
  XS: 'xs',
  SM: 'sm',
  MD: 'md',
};

...
export const Button = ({ shape, size, ...props }) => {
   ...
   // code that compares the shape prop to shapes.ROUNDED for example, 
   ...
   // code that compares the size prop to sizes.XS for example
  ...
};

Button.propTypes = {
  shape: PropTypes.oneOf(Object.values(shapes)),
  size: PropTypes.oneOf(Object.values(sizes)),
};

Button.defaultProps = {
  shape: shapes.ROUNDED,
  size: sizes.SM,
};

It seems like the above way to define prop types confuses react-docgen, and I end up with unusable Storybook controls. I assume it is doing some form of parsing, not some true evaluation that would actually resolve Object.values(sizes) to the array of values that it is. In the UI, the control shows that the possible values are... Object.values(sizes) instead of the actual values, and the drop down that should let me choose an enum value is a list of numbers from 0 to the length of that Object.values(sizes) string (!). It works fine for simple types like PropTypes.bool, but enums are just lost in translation, and I use a lot of enums in my components.

Thanks a lot

@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

@sebastienbarre that sounds like a limitation of react-docgen. You might be able to create a custom handler, perhaps based on https://github.com/reactjs/react-docgen/blob/main/src/handlers/propTypeHandler.ts.

However, we don't have a way for you to use a custom handler, yet. If you want to experiment, you should be able to edit the propTypeHandler within react-docgen in your node_modules, to see if you can accomplish what you need. If so, let us know and we'll work on making the plugin configurable.

This new vite plugin is not configurable like the babel plugin is, but I'm not certain those configurations are necessary in the first place. If we hear from users that it's something they need, we can find a way to make them customizable, but I prefer to avoid extra options if possible.

That didn't take long to be proven wrong. 😆

@sebastienbarre
Copy link

Thanks @IanVS.

If I may ping react-docgen's very own @danez for a hot second, maybe he could share if react-docgen 6.0 would handle situations like these in the future?

Button.propTypes = {
  shape: PropTypes.oneOf(Object.values(shapes)),
  size: PropTypes.oneOf(Object.values(sizes)),
};

@danez
Copy link

danez commented Apr 4, 2022

I'm not sure I can follow, but this should work already in v5.

if you copy the example into the playground (which runs on v6) you can see what happens:

import React, { Component } from 'react';
import PropTypes from 'prop-types';

const shapes = {
  1: "a",
  2: "b", 
};

class MyComponent extends Component {
  render() {
    // ...
  }
}

MyComponent.propTypes = {
  foo: PropTypes.oneOf(Object.values(shapes)),
};

export default MyComponent;

@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

Oh cool. @sebastienbarre would you be willing to open an issue (in this repo) and ideally provide a small reproduction that we can test against, to figure out what's going on here?

@reintroducing
Copy link

reintroducing commented Apr 4, 2022

Hmm, this still does not seem to be working for me. Given the Button example I provided in a different spot in this repo (i forgot where, but thats irrelevant):

import {forwardRef} from 'react';
import PropTypes from 'prop-types';
import {cnb} from 'cnbuilder';
import Loader from '../Loader';
import css from './Button.module.scss';

const Button = forwardRef(
    (
        {
            className,
            testAttr,
            children,
            type,
            variant,
            size,
            block,
            icon,
            iconPosition,
            disabled,
            loading,
            onClick,
            ...attrs
        },
        ref
    ) => (
        <button
            ref={ref}
            className={cnb(
                css.root,
                {[css[variant]]: variant},
                {[css[size]]: size},
                {[css.block]: block},
                {[css[`${iconPosition}Icon`]]: Boolean(icon)},
                className
            )}
            data-test={testAttr}
            type={type}
            disabled={disabled}
            onClick={onClick}
            {...attrs}
        >
            {iconPosition === 'left' &&
                (loading ? (
                    <Loader
                        className={css.leftLoader}
                        testAttr="loader"
                        inline
                    />
                ) : (
                    icon
                ))}
            {children}
            {iconPosition === 'right' &&
                (loading ? (
                    <Loader
                        className={css.rightLoader}
                        testAttr="loader"
                        inline
                    />
                ) : (
                    icon
                ))}
        </button>
    )
);

Button.propTypes = {
    /** Additional class(es) to add to the component. */
    className: PropTypes.string,
    /** The name of the test attribute to associate to the component. */
    testAttr: PropTypes.string,
    /** The markup node to insert into the button. */
    children: PropTypes.node.isRequired,
    /** The type of button that is rendered. */
    type: PropTypes.oneOf(['button', 'submit', 'reset']),
    /** The variant of the button. */
    variant: PropTypes.oneOf([
        'primary-link',
        'neutral-link',
        'destructive-link',
        'primary',
        'secondary',
        'tertiary',
        'neutral',
        'tertiary-neutral',
        'primary-destructive',
        'secondary-destructive',
        'tertiary-destructive',
        'neutral-destructive',
    ]),
    /** The size of the button. */
    size: PropTypes.oneOf(['sm', 'md', 'lg', 'xl', 'xxl']),
    /** Whether the button should display as a block level element. */
    block: PropTypes.bool,
    /** A custom icon (typically one of the Icon components) to show in the button. */
    icon: PropTypes.element,
    /** If an icon is provided, whether it should appear on the left or right side of the button children. */
    iconPosition: PropTypes.oneOf(['left', 'right']),
    /** Whether the button is disabled or not. */
    disabled: PropTypes.bool,
    /** Whether to show a loading animation inside of the button. */
    loading: PropTypes.bool,
    /**
     * The handler to execute when the button is clicked.
     *
     * @param {SyntheticEvent} evt - The React `SyntheticEvent`.
     */
    onClick: PropTypes.func,
};

Button.defaultProps = {
    testAttr: 'button',
    type: 'button',
    variant: 'primary-link',
    size: 'lg',
    iconPosition: 'left',
};

export default Button;

This is the corresponding story:

export const Default = Template.bind({});

Default.args = {
    children: 'Button Label',
};

Here is the output I'm still seeing:
Screen Shot 2022-04-04 at 8 32 13 AM

I pasted my Button code into the playground provided above by @danez (great tool by the way, had no idea it existed) and it did produce the proper output but it's not being reflected in Storybook. I verified that I upgraded to the latest version (i even removed my node_modules and package-lock just to make sure as well) but still don't see those props :\

@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

@reintroducing thanks for the report. Is there any chance you could create a repo that shows this behavior, so we can dig into it and see what's going on? I tried to reproduce it in the examples/react by using forwardRef in our button, but that worked fine.

@sebastienbarre
Copy link

sebastienbarre commented Apr 4, 2022

Thanks @danez for the lightning fast reply. That is indeed what I'm trying to do in Storybook.

@IanVS I'm trying as we speak, but I'm having issues with react-docgen for some reasons. I followed your README to start from a project scratch:

npm create vite@latest # follow the prompts
npx sb init --builder @storybook/builder-vite && npm run storybook

getting:

$ npm install
...
$ npm run storybook                                                                                               

> storybook-builder-vite-react-docgen-repro@0.0.0 storybook
> start-storybook -p 6006

info @storybook/react v6.4.20
info
info => Loading presets
ERR! Error: Cannot find module 'react-docgen/dist/utils'

@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

@sebastienbarre I think you might be missing an npm install before running storybook? No, I think sb init should do that, so I'll try this out and maybe it's a bug. Thanks.

@reintroducing
Copy link

reintroducing commented Apr 4, 2022

@IanVS Here you go:
storybook-vite-example.zip

As you can see, the Default story only has the children, the Primary story has children and variant, but there are many other props that should be showing up. Let me know if you need anything else.

EDIT: Just run npm i and then npm start.

@sebastienbarre
Copy link

@IanVS I did run npm install just to be safe too, and no, that unfortunately wasn't the solution. I'm a bit puzzled on this one.

@sebastienbarre
Copy link

sebastienbarre commented Apr 4, 2022

@IanVS Is build-vite using react-docgen 6.0.0? Just mentioning that because 6.0.1 was released a few hours ago, and it says:

BREAKING CHANGES
* The path inside the npm package was changed from /dist/ to /lib/

Are you maybe explicitly including something from dist/utils that should now be lib/utils?

That seems to be the case:

node_modules/@storybook/builder-vite/dist/plugins/docgen-handlers/actualNameHandler.js
14:const utils_1 = require("react-docgen/dist/utils");

node_modules/@storybook/builder-vite/plugins/docgen-handlers/actualNameHandler.ts
14:import { getNameOrValue, isReactForwardRefCall } from 'react-docgen/dist/utils';

These seem to be in lib/utils now:

$ ls node_modules/react-docgen/lib/utils/getName*
   getNameOrValue.d.ts       getNameOrValue.js

@danez
Copy link

danez commented Apr 4, 2022

I'm actually reverting this as we speak, because there was no real good reason to change the folder name. alpha.2 in a couple of minutes.

EDIT: If you try now and ensure you update react-docgen to alpha.2 it should work again.

@sebastienbarre
Copy link

Alright, thanks a lot @danez -- we are back in business.

@IanVS I created a repo that show the behavior I described: sebastienbarre-forks/storybook-builder-vite-react-docgen-repro

My apologies though, my original example above was a bit too simplified.

I started from the example in the build-vite README:

npm create vite@latest # follow the prompts
npx sb init --builder @storybook/builder-vite && npm run storybook

Then I slightly updated the Button story to use an object for the constants (this commit):

const sizes = {
  SMALL: "small",
  MEDIUM: "medium",
  LARGE: "large",
};

...

Button.propTypes = {
  size: PropTypes.oneOf(Object.values(sizes)),
};

...

Button.defaultProps = {
  size: sizes.MEDIUM,
};

As @danez kindly pointed out, this actually is already handled properly by react-docgen:

image

What doesn't quite work is what I should have clarified earlier (my bad) -- our constants are in a separate file.
The reason for this is that many of our components take props like size, shape, theme, or shade and we want the values for these props to be consistent. However, not ALL of our components support all sizes for example. In other words, we want the values of size to be consistent across all our components, but each component may support only a subset of them. Same for shapes, or shades, etc.

It looks like this (this commit):

In Size.js:

export const Size = {
  EXTRA_SMALL: "extra_small",
  SMALL: "small",
  MEDIUM: "medium",
  LARGE: "large",
  EXTRA_LARGE: "extra_large",
};

and the only change to Button is to use a subset of these sizes:

import { Size } from "./Size.js";

const sizes = {
  SMALL: Size.SMALL,
  MEDIUM: Size.MEDIUM,
  LARGE: Size.LARGE,
};

Here is what it looks like now in Storybook (notice the null):

image

Thank you.

@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

@reintroducing I've traced your issue to #312.

In the meantime, you can set this in your main.js to force react-docgen to be used:

module.exports = {  
  typescript: {
    reactDocgen: 'react-docgen',
  }
};

image

@reintroducing
Copy link

reintroducing commented Apr 4, 2022

@IanVS That actually generates ERR! Error: Cannot find module 'react-docgen/dist/utils'. Do I then also need to install and manage the react-docgen dep? I would have thought this was already installed under the hood, no?

EDIT: I do actually see the dep is already installed, so not sure why i'm seeing that error.

@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

@reintroducing can you try clearing your package-lock.json, deleting your node_modules and then running npm install again? A new version was released that fixes this.

@IanVS
Copy link
Member Author

IanVS commented Apr 4, 2022

@sebastienbarre I took a look at your repro, thanks for that! After the same workaround to force react-docgen to be used (#299 (comment)), I do indeed see that what's being generated by docgen isn't quite right:

Button.__docgenInfo = {
  "description": "Primary UI component for user interaction",
  "methods": [],
  "displayName": "Button",
  "props": {
    /// ...
    "size": {
      "defaultValue": {
        "value": '"medium"',
        "computed": false
      },
      "type": {
        "name": "enum",
        "value": [{
          "value": "null",
          "computed": false
        }, {
          "value": "null",
          "computed": false
        }, {
          "value": "null",
          "computed": false
        }]
      },
      "required": false,
      "description": "How large should the button be?"
    },
    /// ...
  }
};

I think this is probably a limitation of react-docgen, but maybe you can open a feature request there, and perhaps even contribute a PR to add support? It seems like a reasonable request to me, but @danez is the judge of course. ;)

If it can't make its way into react-docgen itself, maybe it can be done in a custom handler (which as I said, we'd need to add support for here).

@reintroducing
Copy link

@reintroducing can you try clearing your package-lock.json, deleting your node_modules and then running npm install again? A new version was released that fixes this.

Rookie mistake, I should have known better, the ole delete it and re-install it :) That totally works, thank you.

@sebastienbarre
Copy link

Thanks.

@danez, do you think the above example is in the realm of what react-docgen could support in the future, or could be made to support? Or would it need some serious refactoring/re-engineering? Unfortunately, my knowledge of TypeScript is very limited at the moment, but I could have a look.

@danez
Copy link

danez commented Apr 5, 2022

Yes totally. I haven't looked at exactly what's going on, but to me this even looks more like a bug than a feature request. The output is not usefull at all. Usually in cases that are not supported the type should be custom, with the raw code attached.
Anyway a new issue with an example would be great. Then I can get back to it later or feel free to dive in and have a look for yourself.

@sebastienbarre
Copy link

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.

DocsPage not populated from JSDoc comments Looks like the Actions addon is not working properly
6 participants