Regression with default value not being selected in Controls column

[Naoto-Ida]

Describe the bug
We've been loving Storybook with our Typescript setup, but after upgrading from 6.2.9 to 6.3.0, we've lost the feature to have SB select the default value in the Controls column.

For example, in the demo files produced with npx sb init, a Button.tsx is created.
Its size prop has a default value of medium, but the RadioControl does not select the radio option labeled medium anymore.

To Reproduce
I have a very simple reproduction repo which simply ran npx sb@next repro with no manual configurations.

System
Environment Info:

System:
OS: macOS 11.4
CPU: (8) x64 Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
Binaries:
Node: 12.20.2 - ~/rg/community/bin/build/nodejs/bin/node
Yarn: 1.22.5 - ~/rg/community/bin/build/yarn/bin/yarn
npm: 6.14.11 - ~/rg/community/bin/build/nodejs/bin/npm
Browsers:
Chrome: 91.0.4472.114
Firefox: 89.0
Safari: 14.1.1

Additional context
I'm not that familiar with SB's codebase, but what I can tell is that the value prop for RadioControl is undefined.
The only time this is not undefined is when the Story supplies it via the args property, i.e.

const Template: ComponentStory<typeof Button> = (args) => <Button {...args />

const Example1 = Template.bind({});

Example1.args = {
  size: 'large'
}

[shilman]

This is the new intended behavior. Please see MIGRATION.md for an explanation and the workaround:

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

[Naoto-Ida]

Thanks for pointing me to the relevant migration doc.

In 6.3 we no longer infer default values and instead set arg values to undefined when unset, allowing the framework to supply the default value.

Could you help me understand what it means my "allowing the framework to supply the default value"?

I'm struggling to understand what necessitated this change in behavior, as we'll need to duplicate default values in the component, as well as the story.

[shilman]

Consider the react component:

const Foo({ input = 'hello' }) => <div>{input}</div>

Now consider two different ways of calling that function:

const input = someAnalysisThatWillHopefullyProduceHelloButMightFail();
<Foo input={input} />

Alternatively:

<Foo input={undefined} />

The former will display 'hello' in most cases, but can also fail in others. The latter will always product 'hello'. That's why we changed the default value handling in 6.3, to avoid the failure cases.

To restore the 6.2 behavior, you can set the value of the arg to the desired input:

export default {
  title: 'Foo',
  component: Foo,
  args: { input: 'hello' }, // <== this line
}

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

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

[Naoto-Ida]

Could this be an opt-in/opt-out feature? I understand that the aim was to let the users of Storybook decide exactly what the default value is to workaround the various ways of passing props and should be selected in the Controls, but I believe the way in which the components created by default via npx sb init pass props, as well as the same way our engineers in my team pass props, are very common patterns within the React community.

[shilman]

@Naoto-Ida nope, sorry. the arg handling code is already quite complex as is, and the new approach is far less bug-prone and also more self-consistent: if you want the control to be set to a value when you navigate to that story, set the arg in the story.

[DaniGuardiola]

@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 :P

Thank you!

[shilman]

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

[mayank99]

const Foo({ input = 'hello' }) => <div>{input}</div>

const input = someAnalysisThatWillHopefullyProduceHelloButMightFail();
<Foo input={input} />

<Foo input={undefined} />

The former will display 'hello' in most cases, but can also fail in others. The latter will always product 'hello'. That's why we changed the default value handling in 6.3, to avoid the failure cases.

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

I'm failing to see how choosing to not infer default values will help with that.

But that's fine, it just seems like a slight breaking change in a minor release. I think the docs could use an improvement at least.

[shilman]

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.

[tmeasday]

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

I don't think there is any good reason why we would set the arg to undefined as opposed to just not setting it. I'm not sure that's intentional; it could be a consequence of various things.

[tmeasday]

Reading the original conversation between @Naoto-Ida and @shilman I feel like there is a misunderstanding somewhere.

The only intended effect of the change in 6.3 was to not show the control with the initial value selected -- it shouldn't affect how the story/component is rendered. If it does, something unusual is happening, like what @DaniGuardiola described. Certaintly the build in "template" stories render the same AFAIK.

@Naoto-Ida can you explain what you meant?

[DaniGuardiola]

@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 :)

[tmeasday]

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.

[Naoto-Ida]

@DaniGuardiola Thanks for sharing this workaround.

The points which @DaniGuardiola listed all apply to us as well.

What we ended up doing to get the original behavior back for behavior 1 and 2 are to do something similar to the following:

// SomeComponent.js
import defaultArgs from './defaultArgs';

const SomeComponent = ({ color }) => (
 //
);

SomeComponent.propTypes = {
  color: PropTypes.oneOf(['red', 'blue', 'green']),
};

SomeComponent.defaultProps = defaultArgs;

// defaultArgs.js

export default {
  color: 'red'
}

// SomeComponent.stories.js

export default {
  component: SomeComponent,
  args: defaultArgs,
}

export const SomeComponentWithBlueColor = (args) => <SomeComponent {...args} />;

SomeComponentWithBlueColor.args = {
  color: 'blue'
};

Note that this example is in JS but we now take a similar approach to our other Storybook which has Typescript (but with default arg values by destructuring props).

[penx]

For what it's worth, we encountered this issue in our Footer component where we were setting a 'Container' component as a default prop (not something we expect Storybook to be concerned with) but Storybook 6.3+ sets this to undefined, causing the component to throw an error.

We also seem to have an issue with storyshots 6.2.9 exhibiting the same issue (although it renders fine in Storybook 6.2.9).

govuk-react/govuk-react#902 (comment)

[tmeasday]

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

[penx]

@tmeasday yes that seems to be what's happening.

Here's an example repo using Storybook 6.2.9 that shows what seems to be the same issue in storyshots:

https://github.com/penx/storyshots-default-props

const Footer = ({ container: Container, a, b }) => <Container>{a} {b}</Container>;

Footer.propTypes = {
  container: PropTypes.elementType,
  a: PropTypes.string,
};

Footer.defaultProps = {
  container: 'div',
  a: 'A',
  b: 'B',
};

https://github.com/penx/storyshots-default-props/blob/master/src/stories/Footer.jsx

Note:

• if you run yarn storybook with 6.2.9 then you see the component render with "A". B is missing as it's not defined in prop types, but 'container' is there so the component doesn't error.
• if you run yarn test then storyshots errors with "React.jsx: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined". This seems to be because container is somehow missing or is getting set to undefined.
• if you upgrade to storybook 6.3.4 then run yarn storybook you see undefined is not an object (evaluating 'el.type.displayName'), again this seems to be because container is missing or set to undefined

[tmeasday]

@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.

[penx]

Thanks @tmeasday, that makes sense now.

[TristanWatanabe]

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?

@tmeasday In our case, the default value column just results in - value for all props even though a default is set via JSDoc comments.

[tmeasday]

Hi @TristanWatanabe - that sounds like a separate issue I guess, docgen is not working properly in your case?

[shilman]

We’re cleaning house! Storybook has changed a lot since this issue was created and we don’t know if it’s still valid. Please open a new issue referencing this one if:

• this is still a problem in SB 7.x
• you can provide a consistent reproduction in 7.x (at https://storybook.new/ or per repro docs).