fix: dont mutate Story prop types to optional

[Bram-Zijp]

Issue:
When using TypeScript and you specify props, the props were converted to optional. I don't think this is a good pattern. One should type cast if they want to go against specification. I don't see any reason to make it Partial in the code base (correct me if I'm wrong, this Partial seems intentional).

What I did

Removed the Partial from the type defs in my node_modules folder and saw proper results. I searched this repo for references to BaseAnnotations and found nothing concerning.

How to test

• Is this testable with Jest or Chromatic screenshots? -> No
• Does this need a new example in the kitchen sink apps? -> No
• Does this need an update to the documentation? -> No

• I don't know what the reasoning behind this Partial is, I don't see a reason to mutate this the Args type to something else. If modules need to alter it, shouldn't they need to be the ones responsible for doing so? Needs to be verified though. This Partial was probably intentional -> Yes

Example

import React from "react";
import { Story } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

const Template: Story<ExampleProps> = (args) => <div {...args}>example</div>;

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

Test.args = {}; // error requireMe is undefined error should be visible here. This makes it easy to spot mistakes when you make em.

[ndelangen]

Both Partial and non-Partial make sense.

The Partial was added intentionally with the mindset that users would likely want to supply a Props interface which is very strict. (all sorts of required fields)

But then do this:

import React from "react";
import { Story } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

const Template: Story<ExampleProps> = (args) => <div {...defaultProps} {...args}>example</div>;

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

Test.args = {}; // no error, because the component will never be called without 'missing' properties (everything is optional)

But a default stricter stance also makes sense...
Right now, we do not allow the user to be strict.

A user could very easily opt-in to all properties being optional by wrapping their type with Partial<> themselves.

I think this is an OK, change. Though it might be a breaking change.

[ndelangen]

IF we were to make this change...
Here's how a user could modify their story to get the old behavior back:

import React from "react";
import { Story } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

- const Template: Story<ExampleProps> = (args) => <div {...defaultProps} {...args}>example</div>;
+ const Template: Story<Partial<ExampleProps>> = (args) => <div {...defaultProps} {...args}>example</div>;

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

Test.args = {}; // no error, because the component will never be called without 'missing' properties (everything is optional)

[shilman]

I think the issue here is that args are cascading. So let's say you have:

interface ExampleProps {
  a: number;
  b: number;
}

You could have stories that look like:

import React from "react";
import { Story, Meta } from "@storybook/react";

interface ExampleProps {
  require1: string;
  require2: string;
}

export default {
  title: 'foo';
  args: { require1: 'haha' }
} as Meta<ExampleProps>

const Template: Story<ExampleProps> = (args) => <div {...args}>example</div>;
export const Test = Template.bind({});
Test.args = {
  require2: 'hoho';
}

Which is valid with the partial type but not valid with the strict type.

[ndelangen]

Yeah the more I think about the more I think the Partial approach makes the most sense as a default.

We could do this:

import React from "react";
import { StrictStory } from "@storybook/react";

interface ExampleProps {
  requireMe: "please";
  fine?: true;
}

const Template: StrictStory<ExampleProps> = (args) => <div {...args}>example</div>;

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

Test.args = {}; // error requireMe is undefined error should be visible here. This makes it easy to spot mistakes when you make em.

[shilman]

Given the problems with this PR, are there cleaner solutions to help make this more strict? @ndelangen @Bram-Zijp

[ndelangen]

As is, this can't be merged. But I'd be open to a StrictStory addition.

[yannbf]

I'm pretty sure that making the Story type strict will break a lot of people's codes. I think Partial makes sense, given how flexible the args are. I'd vouch for a secondary generic parameter in the Story type, with a sole purpose to toggle a strict mode. This way we introduce a desired feature that users can easily opt in:

type StrictModes = 'loose' | 'strict'
interface Story<Args, StrictMode extends StrictModes = 'loose'> {
  args: StrictMode extends 'strict' ? Args : Partial<Args>
}

const PartialTemplate: Story<ExampleProps> = (args) => <div {...args}>example</div>;
const StrictTemplate: Story<ExampleProps, 'strict'> = (args) => <div {...args}>example</div>;

This way people can refer to tsdoc from Story and figure it out:

Here's a playground with a working prototype

Either that or StrictStory type are fine by me!

Edit: thinking more about it, I guess people won't want to have a mix of Story<Props> and Story<props, 'strict'>, so maybe StrictStory type makes more sense and it's easier for people to use.

[Bram-Zijp]

I agree on that @yannbf. I'm lacking time and intel on the repo. Could someone pick this up?

[apdrsn]

Hi Team Storybook :)
What's the status on this feature? I think the StrictStory would make sense. It's a problem that we can't rely on the type checking as it is now :)

[ndelangen]

I think introducing StrictStory would be the best way forwards. Do you agree @yannbf @shilman ?

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2ec5775. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 1 target

• nx run-many --target=prepare --all --parallel=2 --max-parallel=15 -- --optimized

---

Sent with 💌 from NxCloud.

[ndelangen]

So with future/base soon to be merged into next we need to make a decision here @tmeasday @shilman...

• Do we make this strict?
• Do we go with Kasper's new Typescript syntax
• Do we add a StrictStoryFn and a StrictStoryObj interface?

[ndelangen]

@kasperpeulen What should we do with this PR considering your recent and ongoing work around types and strictness?
Can we somehow incorporate some of @Bram-Zijp 's commits so they get a bit of credit? I suspect this PR won't actually land.

I'm sorry about that @Bram-Zijp
The solution wasn't as straight forward, but in the end I think you'll be quite pleased with the hard work @kasperpeulen has put into and the results he'll achieve (soon, I'm sure).

It will likely mean your PR will most likely stay unmerged, and at some point will be closed.
If it does, please do know we very much appreciate your time, energy and expertise in creating this PR. Thank you.

@kasperpeulen I'll leave it to you to decide what to do with this PR, let me know if you need any help to decide or to act upon it.

[Bram-Zijp]

@ndelangen I am happy that it got picked up. I appreciate you want to give me some credit but I don't wanna burden anyone. Therefor I am closing this PR. I am a big fan of Storybook, keep up the good work!

[kasperpeulen]

@Bram-Zijp Follow up PR is here:
#19238