tajo / ladle

🥄 Develop, test and document your React story components faster.
https://www.ladle.dev
MIT License
2.63k stars 93 forks source link

Way to verify that all stories render without errors #515

Open Hurtak opened 1 year ago

Hurtak commented 1 year ago

Is your feature request related to a problem? Please describe.

What happened a bunch of time to me is

  1. I had a working stories for some component
  2. I updated the component by adding new required prop - without it the component crashes
  3. I forget to update the stories
  4. Since the stories use the controls Story<Props> type + .args pattern (that is AFAIK the recommended way to do it in the docs) the story crashes

We do have tsc + eslint + ladle build in our CI pipeline and none of these catches that the story is no longer working

Describe the solution you'd like

2 solutions come to my mind

Additional context

component

export type TestComponentProps = {
  requiredA: () => number;
  requiredB: () => number;
};

export const TestComponent = ({ requiredA, requiredB }: TestComponentProps) => {
  return <h1>a+b = {requiredA() + requiredB()}</h1>;
};

story

export const Test: Story<TestComponentProps> = (props) => <TestComponent {...props} />;
Test.args = {
  requiredA: () => 1,
  // requiredB: () => 2, 
  // ^ if this one is missing, tsc/linter/build passes but the story crashes when rendered
};
tajo commented 1 year ago

I think not applying Partial here would fix this this (threw an error) https://github.com/tajo/ladle/blob/main/packages/ladle/lib/app/exports.ts#L94C5-L94C12

@wojtekmaj any thoughts on this?

wojtekmaj commented 1 year ago

The problem I was aiming to address with Partial is in my opinion bigger than the one reported here, and I don't know if I have a solution to make everyone happy. In reality many stories would change one, maybe two props, leaving others at their defaults, making them pretty good, isolated and readable test cases. Consider the following, if Partial was missing:

export const Test: Story<TestComponentProps> = (props) => <TestComponent requiredB={2} {...props} />;

Test.args = {
  requiredA: () => 1,
  // ^ tsc/linter/build would crash despite the fact in reality it would have rendered just fine
};
tajo commented 1 year ago

should we have Story and StoryRequired types?

wojtekmaj commented 1 year ago

Sounds like a plan!

Hurtak commented 1 year ago

Yea I think the StoryRequired should solve this.

One think to consider might be: the pattern mentioned in https://github.com/tajo/ladle/issues/515#issuecomment-1742446858, that would break if Partial was missing, seems not to be that common (at least by briefly looking at our stories, it is less than 10% of stories), so maybe we could make the Story type strict (without Partial) and introduce StoryPartial for these cases? This would probably be a breaking change, so not sure if it is worth it, but having safer default seems like a good thing to do.

absassi commented 7 months ago

I think it is quite valuable to have some way to render all stories. Missing props is just one possible reason for a story to be broken. It's important is to check the stories in runtime, not just static type checking (if it does both, even better). In my opinion, best way is if this could be done by just creating a test for each story. Not sure how difficult is to deal with support for different test runners though. But if it's done by some separate command like ladle verify as suggested (possibly using Vitest, since Ladle already uses Vite), that's fine too.