Addon-controls: Better handling of Typescript optional properties

[alexbchr]

Is your feature request related to a problem? Please describe.
I am not sure if this is a bug or not, but I put it as a feature request, since I didn't find any documentation on this use case...

The Controls Addon can infer controls from my React component prop types. For example, having the following Typescript props for a component works well:

export interface TextProps {
  text: string
  loading: boolean
}

This correctly renders a text control for the text property and a boolean control for the loading property.

However, let's say I make the props optional (as is the case of most props of React Components):

export interface TextProps {
  text?: string
  loading?: boolean
}

Both controls for both properties are now rendered as a raw text input (which strangely is not a control of type text). They should be rendered as in the first example.

Describe the solution you'd like
Solution could be to handle optional properties as if they were not optionals if the component defines a default value for it.

For example, consider the following component:

export interface TextProps {
  text?: string
  loading?: boolean
}

export const TextProps: React.FC<TextProps> = ({ text = '', loading = false }) => {
  //...
}

Since default values are defined (and are already shown in props table), the controls should act as if undefined was not an actual value and display text and boolean controls for text and loading properties repectively.

Describe alternatives you've considered
The only solution for me right now is to declare the Story argTypes manually for each, which is really not cool, since most Props of all my components are optionals, therefore requiring tedious manual setup in each of my stories.

[shilman]

@alexbchr do you have a public repro of this? Also make sure you are on the latest beta. i tried to repro it in our monorepo, but it behaves the way I'd expect it. Can you take a quick look and let me know if you are seeing something different than this?

Input: https://github.com/storybookjs/storybook/pull/11149/files#diff-7b136f51af3f83ecf482139c745e467d

Output:

[alexbchr]

I am effectively on the latest Beta (26).

Here is my main.ts:

import path from 'path'
import type { StorybookConfig } from '@storybook/core/types'

const tsconfig = path.resolve(__dirname, '../../tsconfig.json')

const storybookConfig: StorybookConfig = {
  stories: ['../../src/components/**/*.stories.tsx'],
  addons: [
    '@storybook/addon-actions/register',
    '@storybook/addon-toolbars',
    '@storybook/addon-docs',
    '@storybook/addon-controls',
  ],
  typescript: {
    check: true,
    checkOptions: { tsconfig },
    reactDocgen: 'react-docgen-typescript',
    reactDocgenTypescriptOptions: {
      shouldExtractLiteralValuesFromEnum: true,
      tsconfigPath: tsconfig,
      propFilter: prop => !/^(testID)$/.test(prop.name),
    },
  },
  webpackFinal: async config => {
    if (!config?.resolve) {
      return config
    }

    config.resolve.alias = {
      ...(config.resolve.alias || {}),
      // Replace react-native dependencies with react-native-web
      'react-native$': 'react-native-web',
      // Replace @storybook/react-native with @storybook/react
      '@storybook/react-native': '@storybook/react',
      // Make react-native-svg work
      'react-native-svg': 'react-native-svg/lib/commonjs/ReactNativeSVG.web',
    }

    return config
  },
}

export default storybookConfig

Here is my component:

import React from 'react'
import { ButtonBasePublicProps } from '../ButtonBase/ButtonBase'
import { SecondaryButton } from './SecondaryButton/SecondaryButton'
import { PrimaryButton } from './PrimaryButton/PrimaryButton'
import { DestructiveButton } from './DestructiveButton/DestructiveButton'
import { BorderlessButton } from './BorderlessButton/BorderlessButton'

export interface ButtonProps extends ButtonBasePublicProps {
  /**
   * Set to true to have a primary button.
   */
  primary?: boolean
  /**
   * Set to true to have a destructive button.
   */
  destructive?: boolean
  /**
   * Set to true to have a borderless button.
   */
  borderless?: boolean
}

/**
 * Button triggering certain user actions.
 */
export const Button: React.FC<ButtonProps> = ({
  primary = false,
  destructive = false,
  borderless = false,
  ...props
}) => {
  if (destructive) {
    return <DestructiveButton {...props} />
  } else if (primary) {
    return <PrimaryButton {...props} />
  } else if (borderless) {
    return <BorderlessButton {...props} />
  } else {
    // Secondary
    return <SecondaryButton {...props} />
  }
}

And here is my output:

[shilman]

@alexbchr Do you have a repo i can look at?

[abbudao]

Hey @shilman, I believe I'm having this same issue!

On top of that, the zero-config Typescript setup is not working for me, as types are not being inferred at all.

Only if I manually configure the typescript field in main.js, the prop table gets correctly filled.

I've never done a repro repo before, so I don't know if it's good enough, but I gave it a shot:
https://github.com/abbudao/undefined-optional-sb6

Let me know if I can help in some other way.

PS: I've been in love with SB 6.0, keep up your awesome work!

[shilman]

@abbudao Thanks so much for the repro, really appreciate it. @alexbchr are you also using CRA?

Here's what I'm seeing. You're using CRA, so Storybook's "zero-config" settings are being replaced by @storybook/preset-create-react-app's zero-config settings, which should line up, but don't.

• Currently, the default behavior appears identical to typescript: { reactDocgen: 'react-docgen' } (it should be react-docgen-typescript)
• We should update the CRA preset to use react-docgen-typescript-plugin to utilize @hipstersmoothie 's excellent work React: Switch react-docgen-typescript-loader to react-docgen-typescript-plugin #11106

@mrmckeb LMK if you can take this or we can pair on it?

[alexbchr]

@shilman Sorry for the delay for the repro, but here it is:
https://github.com/alexbchr/upgraded-giggle

Note that I am not using CRA, but a React Native + Expo setup.

I reproduced the issue with Betas 26 and 28.

Also, I referenced most packages I use in my original repo in the package.json, which include many dependencies for React Native + Expo.

Also, Storybook CLI wasn't able to parse my .storybook/main.ts file (because it is in Typescript), until I added the babel.config.js file with the following content (used by Expo):

module.exports = function (api) {
  api.cache(true);
  return {
    presets: ["babel-preset-expo"],
  };
};

[shilman]

@alexbchr I get the following error trying to install:

error An unexpected error occurred: "https://github.com/expo/react-native/archive/sdk-37.0.0.tar.gz: connect ECONNREFUSED 140.82.114.3:443".

That said, I'm not surprised RN is failing--it's not supported by addon-docs per the compatibility guide.

[alexbchr]

@alexbchr I get the following error trying to install:

error An unexpected error occurred: "https://github.com/expo/react-native/archive/sdk-37.0.0.tar.gz: connect ECONNREFUSED 140.82.114.3:443".

That said, I'm not surprised RN is failing--it's not supported by addon-docs per the compatibility guide.

That's weird, this package directly comes from Expo and should be available since it is hosted on GitHub... Everything works fine for me when installing packages using Yarn. When I access https://github.com/expo/react-native/archive/sdk-37.0.0.tar.gz from Chrome it downloads the tar.gz just fine.

I understand that RN is not supported. This is why I use React Native Web to render components in a webpage and why @storybook/react-native is not referenced. Everything Storybook-related works fine except this single issue.

[shilman]

That’s weird. I’ll clear the cache and try again tomorrow.

[shilman]

@abbudao I released @storybook/preset-create-react-app@3.1.0 with the upgrade, but now it seems to only show the react-docgen behavior. I'm not sure whether this is a bug in the the preset or in react-docgen-typescript-plugin. @mrmckeb @hipstersmoothie would it be possible for one of you to look into this?

[hipstersmoothie]

Is this behavior exhibited in the example in the repo?

[shilman]

@hipstersmoothie Yes. Here is the repro:

1. Clone https://github.com/abbudao/undefined-optional-sb6
2. Install and run storybook & observe the Controls tab
3. Comment out the typescript field in main.js
4. Re-run and observe the Controls have downgraded per @abbudao 's screenshot
5. Upgrade @storybook/preset-create-react-app to 3.1.0 containing feat(cra): move to react-docgen-typescript-plugin presets#149, which makes react-docgen-typescript the default and moves to react-docgen-plugin
6. Observe that no matter the typescript setting in main.js, the downgraded controls are shown

[hipstersmoothie]

This is happening because of #11140. If the project doesn't have typescript installed the react-docgen-typescript-plugin is basically a no-op.

In the reproduction provided the project doesn't have typescript installed, so th plugin does nothing

[shilman]

@hipstersmoothie the project does have typescript in its dependencies (not devDependencies)

[hipstersmoothie]

whoops didn't see that

[shilman]

I'm also seeing similar behavior in @alexbchr's repo, which is a completely different (non-CRA) setup. https://github.com/alexbchr/upgraded-giggle

[hipstersmoothie]

Ok this is probably because we need to provide some default compiler options. When you remove the typescript block the plugin doesn't get loaded with any compiler options and misses these two key setting allowSyntheticDefaultImports and esModuleInterop (which implies allowSyntheticDefaultImports)

Adding the following fixes the loading

[hipstersmoothie]

I'll get a PR up that uses the root tsconfig if no tsconfigPath or compilerOptions is provided

[alexbchr]

@hipstersmoothie I just updated my repo and added the following compiler options in the reactDocgenTypescriptOptions:

compilerOptions: {
  allowSyntheticDefaultImports: true,
  esModuleInterop: true
}

The behavior is still the same for me. The props which can be undefined doesn't appear well.

[shilman]

¡Ay Caramba!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-beta.31 containing PR #11184 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.

[abbudao]

@shilman and @hipstersmoothie, thank you for your hard work! 💪

The default Typescript preset is working automagically now! 🥳

Optional fields are still not hiding their undefined nature though.
I bumped the versions and removed the Typescript settings on main.js on the repro repository. This is the resulting Args Table:

Let me know if I can be of any help.

[alexbchr]

For me it is working now! Note however that if I didn't put the following compilerOptions under typescript.reactDocgenTypescriptOptions, I still had previous behavior (like @abbudao)

compilerOptions: {
  allowSyntheticDefaultImports: true,
  esModuleInterop: true,
}

Thanks a lot for the quick fix, this is gonna make Stories Implementation so much simpler and clearer! 🎉

[shilman]

this one was all @hipstersmoothie -- bravo!!! 👏👏👏

[shilman]

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

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

[inomdzhon]

I use storybook v6 version but problem still exist 😞

I create playground with my config here

Could you look it, please 🙏

[milesj]

FWIW, on latest storybook (6.2 at time of writing), undefined is still coming through for optional props. Have done everything above.

The shouldRemoveUndefinedFromOptional option (https://github.com/styleguidist/react-docgen-typescript#shouldremoveundefinedfromoptional-boolean) fixes it for booleans, but not unions.

Related: styleguidist/react-docgen-typescript#341

[victor-grajski]

I just tested setting shouldRemoveUndefinedFromOptional to true and that cleared the undefined for unions and gave me a toggle for boolean values!

[shilman]

@victor-grajski should we make that part of the default settings in storybook? can you open a PR for that?

[DonnyVerduijn]

I just tested setting shouldRemoveUndefinedFromOptional to true and that cleared the undefined for unions and gave me a toggle for boolean values!

Currently dealing with the same problem. However, in my project, most proptypes are nullable types, because they have default props configured. Can we make these types non nullable too besides making them required?

[victor-grajski]

@shilman sorry i missed your mention! I see shouldRemoveUndefinedFromOptional is preset to true in lib/core-server/src/presets/common-presets.ts, is that what you referring to?

Or did you mean open a PR in @storybook/react-docgen/typescript? I don't see a reference to shouldRemoveUndefinedFromOptional there.