Storybook: Controls: Input type should default to select for unions

[sami616]

When a type of union is inferred, or manually passed by passing options to table,

ie: "primary" | "secondary" | "tertiary"

Would rendering a select field populated with the above items in it result in better out of the box UX? Has this already been considered?

[shilman]

Looks like a bug: https://next--storybookjs.netlify.app/official-storybook/?path=/story/argtypes-typescript--unions

You can see it's doing it properly for PropTypes: https://next--storybookjs.netlify.app/official-storybook/?path=/story/argtypes-proptypes--enums

[sami616]

okay good to know i'm not going mad! likely to be in the release?

[shilman]

yep, hopefully next beta

[shilman]

Egads!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-beta.23 containing PR #11070 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

[sami616]

@shilman This issue was fixed in the above release, however i've just upgraded to .37 and it seems to have been a regression somewhere along the line?

[shilman]

In the meantime we upgraded to react-docgen-typescript-plugin which fixed a lot of these issues, but possibly regressed in a few cases too.

For what it's worth, the examples I linked to above seem to still be working fine in the latest beta.

cc @hipstersmoothie

[hipstersmoothie]

Is there a specific example that shows the regression?

[sami616]

@hipstersmoothie @shilman do you still need a reproducible example for this regression? It seems to have reverted for my entire project, all the unions are now rendering as object fields

[hipstersmoothie]

Can you post a minimal repo?

[sami616]

@hipstersmoothie here you go @hipstersmoothie

https://github.com/sami616/sb-ts-union-regression

[sami616]

@shilman @hipstersmoothie at closer inspection, it looks like this is also affecting other fields too, for exmaple - booleans have stopped rendering as checkboxes in my controls UI too.

[shilman]

@sami616 taking a look now to see if this is due to changes I've made on the storybook side. i'll report back.

[shilman]

@sami616 @hipstersmoothie found at least one issue on the storybook side with undefined enum values (or any enum values that are not a number or string). putting in a fix for it now, but would love to figure out a better solution

[sami616]

Is it worth reopening this issue, or happy to create a new one.

[shilman]

I did more research and I think this is actually an issue with react-docgen-typescript. Will try to create a repro and file an issue there. Re-opening this in the meantime.

[sami616]

ah okay, brill thanks @shilman !

[hipstersmoothie]

I now have time to take a look at this. Set up a repo and i'll fix all issues ASAP

[sami616]

Hi @hipstersmoothie this should show it in action, its not just affecting unions, i noticed other controls like booleans for example aren't rendering as checkboxes.

https://github.com/sami616/sb-ts-union-regression

[hipstersmoothie]

Awesome, thanks!

[hipstersmoothie]

@shilman where does sbType get set?

[shilman]

@hipstersmoothie Sorry it's after midnight here and I don't have time to create a repro for you, but here's what I found while investigating this issue. AFAICT react-docgen-typescript (or the plugin?) behaves inconsistently in a variety of cases.

Here are the variables, and note that these variables combine to make different permutations.

Whether the component is exported as a named export or the default

export const MyComponent = ...

const MyComponent = ...
export default MyComonent;

Types vs. interfaces

type Props = ...

interface Props { ... }

Whether the component declares the type of the props, uses FC, or both

const MyComponent = (props: Props) => ...

const MyComponent: FC<Props> = (props) => ...

const MyComponent: FC<Props> = (props: Props) => ...

Whether the props are required or optional

interface Props {
  bool: boolean;
}

interface Props {
  bool?: boolean;
}

Whether the props have default values in the constructor

const MyComponent = ({ bool = false }: Props) => ...

const MyComponent = ({ bool }: Props) => ...

[shilman]

@hipstersmoothie sbType gets set here: https://github.com/storybookjs/storybook/blob/next/addons/docs/src/lib/docgen/createPropDef.ts#L76

It's a little confusing because a few lines below you'll see one for Typescript. As I recall, that one gets called in the react-docgen Typescript case. react-docgen-typescript outputs data in a format that matches propTypes, so I think its output is actually processed by the propTypes codepath (which also has a few specializations for Typescript). Not 100% on that, since I didn't write all of that code.

[shilman]

Another note on the "combinations of variables" thing above. In @sami616 's case you can see his component is defined a little weirdly:

https://github.com/sami616/sb-ts-union-regression/blob/master/src/Spinner/Spinner.tsx#L12-L19

I always use FC<Props>, and as I recall modifying that will modify the react-docgen-typescript output. Ideally we can create a big test matrix for all of these and verify that the behavior is consistent across different idioms.

[hipstersmoothie]

This is the battle with react-docgen-typescript. There are 1000 ways you could define a component. A good thing though is that it has a great test setup. So if we find a case that doesn't work too well the easiest way to fix it is to add a test file here and notify me!

There really is no sure fire way of getting all the docgen info for component because the flexibility that typescript provides. PropTypes is honestly a much easier problem to solve, as there are only a few ways you can use them.

To get these controls to render as selects I think you would have to use one of the following options

https://github.com/styleguidist/react-docgen-typescript#shouldextractliteralvaluesfromenum-boolean
https://github.com/styleguidist/react-docgen-typescript#shouldextractvaluesfromunion-boolean

I'm gonna keep playing around with it.

[hipstersmoothie]

Ideally we can create a big test matrix for all of these and verify that the behavior is consistent across different idioms.

Give me concise examples and I would be happy to fix those cases in react-docgen-typescript

[hipstersmoothie]

added some setting and I think this is what you want right? @sami616

[hipstersmoothie]

But as I said I'm happy to tackle any case RDT isn't already handling

[sami616]

@hipstersmoothie wow, yes! magic. @shilman should these be defaults?

[shilman]

Awesome guys—thanks a ton!!! I’m going to hit the sack and do a pass at all this tomorrow. @hipstersmoothie You’re a hero for taking this on. @sami616 Thanks for raising the issue. Those should probably be added to the defaults

[sami616]

After having set these options, i'm seeing all the native html attributes that are part of my interface, which isnt really desired - is there a way around this guys?

[hipstersmoothie]

There is a propFilter function.

module.exports = {
  typescript: {
    reactDocgenTypescriptOptions: {
      propFilter(prop) {
        if (prop.parent) {
          return !prop.parent.fileName.includes("@types/react");
        }

        return true;
      },
    },
  },
};

[shilman]

yeah there are already a few default options, and by setting the new options you're overwriting those defaults

https://github.com/storybookjs/storybook/blob/next/lib/core/src/server/config/defaults.js

[sami616]

@shilman ah i see! i've re-added the propfilter option for now, (all those controls were crashing my browser!) can these be merged into the default object instead for custom setups?

[shilman]

@hipstersmoothie @sami616 when I add shouldRemoveUndefinedFromOptional: true I get the following error in cra-ts-kitchen-sink, both on CI and also on my local dev machine:

ERROR in ./src/stories/docgen-tests/DocgenTS.tsx
Module build failed (from /Users/shilman/projects/baseline/storybook/node_modules/react-docgen-typescript-loader/dist/index.js):
ValidationError: react-docgen-typescript-loader

  Options Validation Error

  options['shouldRemoveUndefinedFromOptional']  is an invalid additional property

I'm running [email protected] and and [email protected], both of which are the latest versions.

UPDATE: It looks like we're blocked on this: https://github.com/strothj/react-docgen-typescript-loader/pull/98

@hipstersmoothie do you have any way to expedite this? Or is there some workaround that I don't know about?

[hipstersmoothie]

I thought storybook stopped using that. This issue doesn’t exist in react-docgen-typescript-plugin

[hipstersmoothie]

I can take a look after dinner (around 1.5 hours)

[hipstersmoothie]

Ehh a little tired. First thing tomorrow!

[shilman]

Storybook no longer uses RDTL directly. But AFAICT react-docgen-typescript-plugin uses a function from react-docgen-typescript-loader

[hipstersmoothie]

oh you're right. that shouldn't effect the actual docgen though. will investigate soon!

[sami616]

@shilman @hipstersmoothie thanks guys! This is awesome! 🤙🏼

[shilman]

NVM, it looks like a configuration issue in the monorepo example -- old version of preset-cra and preset-cra's RDTP dep is out of date... Will keep you guys posted!

[shilman]

Huzzah!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-rc.3 containing PR #11503 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

[hipstersmoothie]

Is it all good now?

[sami616]

@hipstersmoothie seems good to me

[wengsj11]

Is this issue fixed? I'm getting the same problem here

[wengsj11]

I'm using the latest version v6.0.3

[shilman]

@wengsj11 can you open a new issue with a minimal repro?

[wengsj11]

@wengsj11 can you open a new issue with a minimal repro?

My problem solved after using the new sb cli, I guess that's the reason. 😂