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 default args aggregation in StoryStore. #12992

Closed
wants to merge 3 commits into from

Conversation

vskh
Copy link

@vskh vskh commented Nov 3, 2020

Issue: #12540

TL;DR

I faced a problem causing storyshots to fail with 0 tests found which is caused by undefined/null argument passed into one of the stories.

Symptoms

Snapshot test run fails with message:

 FAIL  src/storyshots.test.ts
  ● Test suite failed to run

    storyshots found 0 stories

      1 | import testStorySnapshots from "@storybook/addon-storyshots";
      2 | 
    > 3 | testStorySnapshots();
        | ^

      at testStorySnapshots (node_modules/@storybook/addon-storyshots/dist/api/index.js:108:15)
      at Object.<anonymous> (src/stoshots.test.ts:3:1)

The error is confusing and does not give any hints as the root cause. Storybook works fine and is not affected.

Sample repo that demonstrates an issue can be found here: https://github.com/vskh/storybook-storyshots-found-0-stories

Root cause

Passing in explicitly undefined argument to the story will break storyshots stories detection, e.g.:

export const Default: Story = args => ...;
Default.args = {
    prop: null
};

In StoryStore, ArgTypes inference in this case is returning undefined entry which then fails to decompose the way that default arguments aggregation expects (see the actual fix).

Retrospectively, now that I know the cause it seems to be not a big deal and rather me doing weird thing instead of omitting an argument (omitting makes it work fine) but I spent few hard hours trying to figure out how two packages with seemingly same configuration exhibit different behaviour so I thought to submit a fix if someone ever hits the same.

What I did

  • fixed default arguments aggregation to handle this case
  • added unit test

How to test

jest lib/client-api/src/story_store.ts

@vskh vskh changed the title Fix default args aggregation and add UT. Fix default args aggregation in StoryStore. Nov 3, 2020
@shilman shilman added the core label Nov 3, 2020
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this issue here that argType inference on arg: undefined is itself argType: undefined?

Perhaps that itself is the issue here? WDYT @shilman? What should the argType be?

Thanks for tracking it down @vskh. I'm not sure you are wrong to set the arg to undefined.

lib/client-api/src/story_store.ts Outdated Show resolved Hide resolved
@shilman
Copy link
Member

shilman commented Nov 5, 2020

I wonder if this fix will remove the need for argType?.defaultValue: #13029

@vskh
Copy link
Author

vskh commented Nov 6, 2020

Yes, that is what happening per my understanding. You can have a look at this line:

if (arg !== null && typeof arg !== 'undefined') {

@vskh
Copy link
Author

vskh commented Nov 6, 2020

Oops, did not see the last comment. Yeah, it should help. I guess my PR is not needed then?

@stale
Copy link

stale bot commented Dec 25, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Dec 25, 2020
@vskh vskh closed this Jan 14, 2021
@vskh vskh deleted the fix-defaultArgs-aggregation branch January 14, 2021 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants