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

Way to verify that all stories render without errors #515

Open
Hurtak opened this issue Oct 1, 2023 · 6 comments
Open

Way to verify that all stories render without errors #515

Hurtak opened this issue Oct 1, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@Hurtak
Copy link

Hurtak commented Oct 1, 2023

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

  • have some new command (like ladle verify) that would actually render all of the stories and report if some of them render error?
  • change something with how the story is defined or its types, so that this would get caught on type level?

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
};
@Hurtak Hurtak added the needs triage needs to be reviewed label Oct 1, 2023
@Hurtak Hurtak changed the title Way to verify that all stories render without errors when using args Way to verify that all stories render without errors Oct 1, 2023
@tajo tajo added enhancement New feature or request and removed needs triage needs to be reviewed labels Oct 2, 2023
@tajo
Copy link
Owner

tajo commented Oct 2, 2023

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
Copy link
Contributor

wojtekmaj commented Oct 2, 2023

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
Copy link
Owner

tajo commented Oct 2, 2023

should we have Story and StoryRequired types?

@wojtekmaj
Copy link
Contributor

Sounds like a plan!

@Hurtak
Copy link
Author

Hurtak commented Oct 6, 2023

Yea I think the StoryRequired should solve this.

One think to consider might be: the pattern mentioned in #515 (comment), 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
Copy link

absassi commented Apr 18, 2024

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.

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

No branches or pull requests

4 participants