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/error handling #10659

Merged
merged 26 commits into from
May 11, 2020
Merged

Fix/error handling #10659

merged 26 commits into from
May 11, 2020

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented May 5, 2020

Issue: #10577 #10664

when there's an error in one of the stories initially, the preview & sidebar will go in a perpetual loading-state & error doesn't show up in the preview

What I did

  • made sure that if an error occurs storybook preview renders an error.
  • Re-use the UI from refs to display the error in sidebar
  • Add the error that occurred to setStories event

@ndelangen ndelangen added this to the 6.0 milestone May 5, 2020
@ndelangen ndelangen requested review from tmeasday, shilman and yannbf May 5, 2020 19:02
@ndelangen ndelangen requested review from alterx and igor-dv as code owners May 5, 2020 19:02
@ndelangen ndelangen self-assigned this May 5, 2020
@ndelangen
Copy link
Member Author

This is what an error now looks like when loading stories fails:
Screenshot 2020-05-06 at 09 22 38
Screenshot 2020-05-06 at 09 23 43

This is what it looks like when a story-file is loaded successfully, but the story throws an error being rendered:
Screenshot 2020-05-06 at 09 21 39

lib/ui/src/components/preview/preview.tsx Outdated Show resolved Hide resolved

if (this._channel) {
this._channel.emit(Events.SET_STORIES, {
v: 1,
Copy link
Member

Choose a reason for hiding this comment

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

magic number?

lib/api/src/tests/stories.test.js Outdated Show resolved Hide resolved
lib/ui/src/components/sidebar/Sidebar.tsx Show resolved Hide resolved
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.

I'm OK with the idea of including errors in SetStoriesPayload (if we need to know about it on the manager side).

But I don't like the changed event behaviour. Let's avoid incremental changes to events like this, that's how we ended up with the complex mess of incoherent events we had before.

Let's be careful about what events are emitted when and document things. I tried to do that already although it could be more comphrehensive I think: https://github.com/storybookjs/storybook/blob/a9fc57fe490e5befcb4bdd4f77f6897d283d8263/lib/core/README.md#events-on-startup

lib/api/src/lib/stories.ts Outdated Show resolved Hide resolved
lib/api/src/modules/refs.ts Show resolved Hide resolved
lib/api/src/modules/stories.ts Outdated Show resolved Hide resolved
lib/client-api/src/config_api.ts Outdated Show resolved Hide resolved
lib/client-api/src/story_store.test.ts Outdated Show resolved Hide resolved
lib/client-api/src/story_store.ts Outdated Show resolved Hide resolved
lib/client-api/src/story_store.ts Outdated Show resolved Hide resolved
Co-authored-by: Tom Coleman <tom@thesnail.org>
@yannbf
Copy link
Member

yannbf commented May 6, 2020

I don't have 100% context so can't help that much in code, but I ran the official storybook in a few scenarios:
1 - with a wrong glob in main.js:

stories: ['../../lib/ui/src/**/Wrong.stories.(js|tsx|mdx)']

2 - with a correct glob but everything commented out in the story:

stories: ['../../lib/ui/src/**/Sidebar.stories.(js|tsx|mdx)']

3 - with a correct glob, exporting a single story but not default exporting anything

Scenarios 1, 2 and 3 leave me in this screen:
image

4 - with a correct glob + the file only has a throw new Error('error!!!');
5 - with a correct glob + the file has a story that throws an error

Scenarios 4 and 5 work as you mentioned in the PR.

And a small observation, that you can ignore if you want, the component /components/sidebar/RefBlocks -> ErrorBlock has the following text:

Ow now! something went wrong loading this storybook

Shouldn't it be

Oh no! Something went wrong loading this storybook.

@ndelangen
Copy link
Member Author

I'll investigate the scenarios:

  • all stories load fine, but they don't export/define any stories
    should show a message in the sidebar
  • story-files are missing (glob matches nothing)
    should show a warning in terminal?

@ndelangen ndelangen requested a review from tmeasday May 7, 2020 15:30
@ndelangen
Copy link
Member Author

Here's what happens if a ref/frame results in a successful SET_STORIES, but the contents are empty:

Screenshot 2020-05-07 at 17 31 45

@ndelangen ndelangen requested a review from shilman May 7, 2020 22:12
@tmeasday
Copy link
Member

tmeasday commented May 8, 2020

Maybe we can make a better UI for the no stories case? WDYT?

@ndelangen
Copy link
Member Author

Screenshot 2020-05-08 at 20 35 56

new UI

@tmeasday
Copy link
Member

Looking good @ndelangen. I was actually thinking something similar in the preview pane. Wdyt?

@tmeasday
Copy link
Member

@domyen want to weigh in?

@ndelangen ndelangen merged commit 1396928 into next May 11, 2020
@ndelangen ndelangen deleted the fix/error-handling branch May 11, 2020 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants