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

Optional values get appended undefined in Addon-docs prop table when using typescript #9395

Closed
vimtor opened this issue Jan 12, 2020 · 17 comments

Comments

@vimtor
Copy link

vimtor commented Jan 12, 2020

Describe the bug
When using addon:docs prop table for React tsx components optional props marked with ? in typescript get appended undefined in the table.

To Reproduce

  1. Follow this guide
  2. Run yarn storybook and see the result.

image

Expected behavior
Props marked as optional don't need the undefined.

System:
System:
OS: Linux 5.0 Ubuntu 18.04.3 LTS (Bionic Beaver)
CPU: (8) x64 Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
Binaries:
Node: 10.18.0 - /usr/bin/node
Yarn: 1.21.1 - /usr/bin/yarn
npm: 6.13.4 - /usr/bin/npm
Browsers:
Chrome: 79.0.3945.88
Firefox: 71.0
npmPackages:
@storybook/cli: ^5.3.0 => 5.3.0

@mrmckeb
Copy link
Member

mrmckeb commented Jan 12, 2020

Hi @papeloto, I feel this is probably expected behaviour.

Optional means it can be undefined - I don't think Docs has any other way to show optional. Is that correct @shilman?

@mrmckeb mrmckeb self-assigned this Jan 12, 2020
@shilman
Copy link
Member

shilman commented Jan 12, 2020

Required props have a red star next to them. Optional props don't. In that sense the | undefined is redundant. However, I'm not sure it's a bad thing? I welcome a hard argument one way or the other :) cc @patricklafrance

@mrmckeb
Copy link
Member

mrmckeb commented Jan 12, 2020

Yes, it's definitely correct that it can be undefined...

@vimtor
Copy link
Author

vimtor commented Jan 12, 2020

I think is both correct and redundant. Maybe more redundant than correct since when you add more props you get something like this.

image

Besides looking buggy is pretty useless because you don't need to know that undefined is an option since you will never pass it to a prop. I mean, you will never do something like this:

<Button color={undefined} />

That being said, a way to filter the props would be nice however, both the rows that are being displayed and the contents.

@mrmckeb
Copy link
Member

mrmckeb commented Jan 13, 2020

@papeloto that's not strictly true. Users may opt to do something like this:

<Button color={() => someThing ? 'red' : undefined} />

But I also agree that we could get rid of undefined. Again, we'd definitely welcome a PR here if you're interested? If not, I can take a look this week. I'm not sure if this work would be better as a part of Docs or the docgen loader (@shilman - what do you think)?

@vimtor
Copy link
Author

vimtor commented Jan 13, 2020

Oops, I didn't consider that case.
First, I will try to solve #9394, so if you can take a look at this 😄

@mrmckeb
Copy link
Member

mrmckeb commented Jan 13, 2020

NP, I'll talk to @shilman about it.

@patricklafrance
Copy link
Member

patricklafrance commented Jan 13, 2020

I agree that "undefined" is redundant. I planned to remove it, some of the code is already in SB https://github.com/storybookjs/storybook/blob/next/addons/docs/src/lib/docgen/typeScript/createType.ts

Haven't had the time yet to plug in in the transformation pipeline.

(when we switch to react-docgen 5.* to handle TypeScript it will be plugged automatically)

@shilman
Copy link
Member

shilman commented Jan 13, 2020

Cool let's plan to just deal with it in the react-docgen@5 upgrade then. 🙏

@shilman
Copy link
Member

shilman commented Jan 13, 2020

@patricklafrance check your discord messages 😘

@vimtor
Copy link
Author

vimtor commented Jan 21, 2020

This has been solved so far, so thank you so much and great work! ❤️

@AsasInnab
Copy link

@vimtor Where has this been solved? I still have this issue

@gabrielcarneiro97
Copy link

@AsasInnab have you found the solution? I'm having this same issue

@NickSevens
Copy link

I'm having the same issue. Storybook v6.3.2.
Is there a fix/workaround for this?

My main.js looks like:

const configureWebpack = require("...");

module.exports = {
	addons: ["@storybook/addon-links", "@storybook/addon-essentials"],
	webpackFinal: (config) => configureWebpack({ config }),
	stories: [...],

	typescript: {
		check: false,
		checkOptions: {},
		reactDocgen: "react-docgen-typescript",
		reactDocgenTypescriptOptions: {
			shouldExtractLiteralValuesFromEnum: true,
			propFilter: (prop) => (prop.parent ? !/node_modules/.test(prop.parent.fileName) : true)
		}
	}
};

@MerlinMason
Copy link

I think beyond the aesthetic concerns it's can cause components to render incorrectly, see the following example (v6.4.8):

Foo.tsx

import React from "react";

interface Props {
    myProp?: "a" | "b";
}

const Foo: React.FC<Props> = ({ myProp = "a" }) => {
    console.log(myProp, typeof myProp);

    if (myProp === "a") return <>A</>;
    return <>B</>;
};

export default Foo;
export type { Props };

Foo.stories.tsx

import { ComponentStory, ComponentMeta } from "@storybook/react";

import Foo from "./Foo";

export default {
    title: "Components/Foo",
    component: Foo,
} as ComponentMeta<typeof Foo>;

const Template: ComponentStory<typeof Foo> = (args) => <Foo {...args} />;

export const A = Template.bind({});
A.args = { myProp: "a" };
export const B = Template.bind({});
B.args = { myProp: "b" };
export const WithDefaultProps = Template.bind({});

Output
image

Console
image

In this case myProp is being passed in as a string "undefined" which is causing the component not to use the default value for myProp (a).

@shilman
Copy link
Member

shilman commented Dec 9, 2021

@MerlinMason what version are you on? pretty sure this got fixed at some point

@MerlinMason
Copy link

@shilman this was v6.4.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants