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

fix: Explicitly allow undefined type for table.type.summary #94

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

Hyzual
Copy link

@Hyzual Hyzual commented Jun 11, 2024

Closes storybookjs/storybook#27129

Allowing explicitly undefined as type for table.type.summary lets Story authors disable the automatic type summary while having strict TypeScript options. With exactOptionalPropertyTypes enforced, using undefined as a type was forbidden, as TypeScript would raise an error. With this change, it should be allowed.

Closes storybookjs/storybook#27129

Allowing explicitly undefined as type for table.type.summary lets Story
authors disable the automatic type summary while having strict
TypeScript options. With exactOptionalPropertyTypes enforced, using
undefined as a type was forbidden, as TypeScript would raise an error.
With this change, it should be allowed.
@dcneiner
Copy link

Did you temporarily turn off exactOptionalPropertyTypes and see if passing undefined behaved like you expect? I don't have that setting turned on, and when I pass undefined for summary, it defaults back to the automatic type definition.

I then tried null as any and that worked as expected. So I'd think we'd need to allow null instead of undefined for this to actually fix the issue.

Maybe I'm missing something else?

@Hyzual
Copy link
Author

Hyzual commented Jun 24, 2024

Yes, we turned off exactOptionalPropertyTypes for the Storybook part of our repository. Writing the following does remove the automatic type definition like we wanted to in Storybook:

table: {
  type: { summary: undefined },
},

We used to pass null instead of undefined, but since the upgrade to Storybook v8.1, the typechecking would not allow it and so we switched to undefined which had the same result for us.

@dcneiner
Copy link

Thank you @Hyzual – I'll see what else I may have different or what I might have missed.

@dcneiner
Copy link

OK, so finally got a chance to test null vs undefined more. I'm not sure why this works for you, but even with a brand new project it only omitted the type when I used null

I took the following steps:

  1. Create a new vite app (npm create vite@latest story-demo -- --template react-ts)
  2. Run npx storybook@latest init
  3. Tried to edit the generated Button.stories.ts to remove the default type for the size prop.

undefined did not work

It still showed union as the type.

const meta = {
  ...
  argTypes: {
    ...
    size: {
      table: { type: { summary: undefined } }
    }
  },
 ...
} satisfies Meta<typeof Button>;
Summary undefined

null worked

Though of course it produces a typescript error:

    ...
    size: {
      table: { type: { summary: null } }
    }
    ...
Summary null

So I believe we the updated type should be:

type?: { summary?: string | null; detail?: string };

@Hyzual
Copy link
Author

Hyzual commented Jun 25, 2024

😲 ok. I'm fine with either undefined or null to be honest

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

Successfully merging this pull request may close these issues.

[Bug]: summary: undefined type issue
2 participants