storybook: Regression with default value not being selected in Controls column

@penx 👋 - so the issue here is that it is setting it to undefined rather than not setting it at all?

@shilman would it be possible to just not pass any value at all to the props? The current behavior messes with our setup:

• We have completely typed props.
• We also have docs for the defaults in the form of JSDoc comments.
• Example of the above:

export interface AvatarProps {
  /**
   * The visual style of the avatar.
   * @default "user"
   */
  variant?: "user" | "entity";

  /**
   * The avatar size.
   * @default "medium"
   */
  size?: "xs" | "small" | "medium" | "large" | "xl" | "2xl";
}

• Then, to enforce the default and still have good type safety, we use this pattern:

const DEFAULT_PROPS = {
  variant: "user",
  size: "medium",
  isButton: false,
} as const;

function Avatar(
  props: AvatarProps
): ReactElement {
  const propsWithDefaults = { ...DEFAULT_PROPS, ...props };
  // ...
}

This allows us to have:

1. Full type safety.
2. Documentation on default values.
3. A propsWithDefaults object that automatically gets the resolved types for each props, potentially with defaults. E.g. instead of 'user' | 'entity' | undefined the type gets resolved to 'user' | 'entity' which is great since that’s the actual type due to the default, and undefined is not an option.
4. Barely any boilerplate, which is great given how cumbersome it usually is to implement defaults with typescript on a React component.

This worked great for earlier versions of storybook, but now I have two issues:

• The defaults stopped working. They don’t show up in their column on the args table.
• Due to storybook passing an object with all props set to undefined, the object spread (which will accept any own property, no matter the value) is replacing the default props with undefined.

This is an awkward situation, because if you look at the types, the component always expects some props to have a value (for example the variant prop above), but it comes with undefined which typescript doesn’t even contemplate.

Doesn’t it suffice just not setting the props at all? Otherwise, this will force us to either not upgrade or change our whole codebase for a storybook quirk 😕

By the way, I’ve also realized that this is only happening the second time a component is loaded in storybook. The first time, it’s fine. This was one of the biggest head-scratchers I initially found 😛

Thank you!

@tmeasday thanks for your input, I understand better what’s happening now.

Just to be clear, here’s the 3 related behaviors I’ve identified on 6.3 regarding defaults:

1. Defaults are not detected from JSDoc comments, and/or not documented in the args table.
2. Defaults are not sent to the rendered story.
3. On second render, the story receives a props object that has (besides the right values for actually selected prop controls) all unselected properties set to undefined.

I understand that behavior 2 is intended and I understand and appreciate the goal.

However, the other two are problematic in my opinion:

• Behavior 1 seems like a regression to me. Being able to document my props using the standard method (JSDoc), and it showing up as part of the documentation just makes sense to me, since, for instance, the description is getting inferred from it anyway.
• Behavior 3 seems to be a bug, as you (@tmeasday) pointed out. Especially if you’re using the object spread or Object.assign to handle defaults, since it doesn’t care about nullish values, and just replaces properties as long as they are set. Of course, this is an edge case that could happen in production if you explicitly set a prop as undefined, but it is very unlikely. In fact, typescript doesn’t even contemplate it, as I described in my earlier comment.

I can attempt to create a repro of behaviors 1 and 3 if that helps. If these behaviors are confirmed, would you consider both to be bugs? Or is anything intentional that I’m missing, especially regarding behavior 3?

Thank you 😃

@penx in your example, your story is defined:

https://github.com/penx/storyshots-default-props/blob/5178570178dafead76bf67a06b7cfe7ea25387ed/src/stories/Footer.stories.jsx#L8

(export const Default = Footer;)

which is wrong (it should be export const Default = (args) => <Footer {...args} />;)

If you make that change, then

(a) in 6.2.9 the component renders correctly with all default values. (b) in 6.3.4 the component also renders correctly with all default values.

In (b) I can see the undefined values getting passed in. For vanilla React this isn’t actually a problem but I agree it is a bug.

Behavior 3 seems to be a bug,

I agree, perhaps you could open a specific issue for this and we can take a look at it.

Defaults are not detected from JSDoc comments, and/or not documented in the args table.

Behavior 1 seems like a regression to me. Being able to document my props using the standard method (JSDoc), and it showing up as part of the documentation just makes sense to me, since, for instance, the description is getting inferred from it anyway.

They should show up in the table, in the “default value” column, just not in the “control” column. Is that not the case in your examples?

Our thinking is that we could probably make it clearer that this what’s happening (that the control isn’t showing anything because we are using the default), but we aren’t going go back to trying to get proper values for args from strings (such as JSDoc comments) as it is too unreliable.

Wouldn’t that function return undefined, and the component would use default value (‘hello’), so both snippets should always display hello?

No, the function could return some arbitrary string, like the name of a variable in your source file.

I think the docs could use an improvement at least.

Here is the documentation: https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#deprecated-argtypedefaultvalue

PRs are very welcome if you have specific ideas on how to make this more clear.

@DaniGuardiola Yes I think that could be a possibility; I’m just not sure about the impact of that change. @tmeasday WDYT?