From 80c96a972f23009758e2f1fc41ad3376383f4bb9 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 6 Aug 2018 17:16:02 +1000 Subject: [PATCH 1/5] Emit messages when stories fail to render --- examples/official-storybook/stories/core.stories.js | 7 +++++++ lib/core-events/index.js | 2 ++ lib/core/src/client/preview/start.js | 5 +++++ 3 files changed, 14 insertions(+) diff --git a/examples/official-storybook/stories/core.stories.js b/examples/official-storybook/stories/core.stories.js index 856042ee22be..e499b387d413 100644 --- a/examples/official-storybook/stories/core.stories.js +++ b/examples/official-storybook/stories/core.stories.js @@ -31,3 +31,10 @@ const increment = () => { storiesOf('Core|Events', module).add('Force re-render', () => ( )); + +storiesOf('Core|Errors', module) + .add('story throws exception', () => { + throw new Error('error'); + }) + // Story does not return something react can render + .add('story errors', () => null); diff --git a/lib/core-events/index.js b/lib/core-events/index.js index c1cbe2ea8819..08e6a0535035 100644 --- a/lib/core-events/index.js +++ b/lib/core-events/index.js @@ -10,4 +10,6 @@ module.exports = { FORCE_RE_RENDER: 'forceReRender', REGISTER_SUBSCRIPTION: 'registerSubscription', STORY_RENDERED: 'storyRendered', + STORY_ERRORED: 'storyErrored', + STORY_THREW_EXCEPTION: 'storyThrewException', }; diff --git a/lib/core/src/client/preview/start.js b/lib/core/src/client/preview/start.js index 613692b90e5d..29cbccad0cc3 100644 --- a/lib/core/src/client/preview/start.js +++ b/lib/core/src/client/preview/start.js @@ -43,14 +43,19 @@ function showErrorDisplay({ message, stack }) { document.body.classList.add(classes.ERROR); } +// showError is used by the various app layers to inform the user they have done something +// wrong -- for instance returned the wrong thing from a story function showError({ title, description }) { + addons.getChannel().emit(Events.STORY_ERRORED, { title, description }); showErrorDisplay({ message: title, stack: description, }); } +// showException is used if we fail to render the story and it is uncaught by the app layer function showException(exception) { + addons.getChannel().emit(Events.STORY_THREW_EXCEPTION, exception); showErrorDisplay(exception); // Log the stack to the console. So, user could check the source code. From 28f10cb71525034d7f792d83714ab9457646190d Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 9 Aug 2018 11:45:47 +1000 Subject: [PATCH 2/5] Added a test file for the start function --- lib/core/src/client/preview/start.test.js | 101 ++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 lib/core/src/client/preview/start.test.js diff --git a/lib/core/src/client/preview/start.test.js b/lib/core/src/client/preview/start.test.js new file mode 100644 index 000000000000..df11855a26b4 --- /dev/null +++ b/lib/core/src/client/preview/start.test.js @@ -0,0 +1,101 @@ +import addons from '@storybook/addons'; +import Events from '@storybook/core-events'; +import { document } from 'global'; + +import start from './start'; + +jest.mock('@storybook/client-logger'); +jest.mock('@storybook/addons'); +jest.mock('global', () => ({ + navigator: { userAgent: 'browser' }, + window: { + addEventListener: jest.fn(), + location: { search: '' }, + history: { replaceState: jest.fn() }, + }, + document: { + addEventListener: jest.fn(), + getElementById: jest.fn().mockReturnValue({}), + body: { classList: { add: jest.fn(), remove: jest.fn() } }, + documentElement: {}, + }, +})); + +function mockEmit() { + const emit = jest.fn(); + addons.getChannel.mockReturnValue({ emit }); + + return emit; +} + +it('renders nopreview when you have no stories', () => { + const emit = mockEmit(); + + const render = jest.fn(); + + start(render); + + expect(render).not.toHaveBeenCalled(); + expect(document.body.classList.add).toHaveBeenCalledWith('sb-show-nopreview'); + expect(emit).toHaveBeenCalledWith(Events.STORY_RENDERED); +}); + +it('calls render when you add a story', () => { + const emit = mockEmit(); + + const render = jest.fn(); + + const { clientApi, configApi } = start(render); + + emit.mockReset(); + configApi.configure(() => { + clientApi.storiesOf('kind', {}).add('story', () => {}); + }, {}); + + expect(render).toHaveBeenCalled(); + expect(emit).toHaveBeenCalledWith(Events.STORY_RENDERED); +}); + +it('emits an exception and shows error when your story throws', () => { + const emit = mockEmit(); + + const render = jest.fn().mockImplementation(() => { + throw new Error('Some exception'); + }); + + const { clientApi, configApi } = start(render); + + emit.mockReset(); + document.body.classList.add.mockReset(); + configApi.configure(() => { + clientApi.storiesOf('kind', {}).add('story', () => {}); + }, {}); + + expect(render).toHaveBeenCalled(); + expect(document.body.classList.add).toHaveBeenCalledWith('sb-show-errordisplay'); + expect(emit).toHaveBeenCalledWith(Events.STORY_THREW_EXCEPTION, expect.any(Error)); +}); + +it('emits an error and shows error when your framework calls showError', () => { + const emit = mockEmit(); + + const error = { + title: 'Some error', + description: 'description', + }; + const render = jest.fn().mockImplementation(({ showError }) => { + showError(error); + }); + + const { clientApi, configApi } = start(render); + + emit.mockReset(); + document.body.classList.add.mockReset(); + configApi.configure(() => { + clientApi.storiesOf('kind', {}).add('story', () => {}); + }, {}); + + expect(render).toHaveBeenCalled(); + expect(document.body.classList.add).toHaveBeenCalledWith('sb-show-errordisplay'); + expect(emit).toHaveBeenCalledWith(Events.STORY_ERRORED, error); +}); From d15db5afac48f75b8e15ac9474fb5303813494ac Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 9 Aug 2018 11:50:48 +1000 Subject: [PATCH 3/5] Don't use the theme provider for error stories @ndelangen it might make more sense to only apply it to "Components|..." story, I think I am right in saying these are the only ones where it matters? --- examples/official-storybook/config.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/examples/official-storybook/config.js b/examples/official-storybook/config.js index 7ba496088485..ef9842a7e621 100644 --- a/examples/official-storybook/config.js +++ b/examples/official-storybook/config.js @@ -18,7 +18,14 @@ setOptions({ theme: themes.dark, }); -addDecorator(story => {story()}); +addDecorator( + (story, { kind }) => + kind === 'Core|Errors' ? ( + story() + ) : ( + {story()} + ) +); configureViewport({ viewports: { From deef786c2b50b2aa1240b5c28bcc8e201a7d3c3b Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 9 Aug 2018 12:27:54 +1000 Subject: [PATCH 4/5] Skip error stories in storyshots --- .../official-storybook/stories/core.stories.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/examples/official-storybook/stories/core.stories.js b/examples/official-storybook/stories/core.stories.js index e499b387d413..4b372a00f0c3 100644 --- a/examples/official-storybook/stories/core.stories.js +++ b/examples/official-storybook/stories/core.stories.js @@ -3,6 +3,7 @@ import { storiesOf, addParameters } from '@storybook/react'; import addons from '@storybook/addons'; import Events from '@storybook/core-events'; import { Button } from '@storybook/components'; +import { navigator } from 'global'; const globalParameter = 'globalParameter'; const chapterParameter = 'chapterParameter'; @@ -32,9 +33,13 @@ storiesOf('Core|Events', module).add('Force re-render', () => ( )); -storiesOf('Core|Errors', module) - .add('story throws exception', () => { - throw new Error('error'); - }) - // Story does not return something react can render - .add('story errors', () => null); +// Skip these stories in storyshots, they will throw -- NOTE: would rather do this +// via a params API, see https://github.com/storybooks/storybook/pull/3967#issuecomment-411616023 +if (navigator && navigator.userAgent && !(navigator.userAgent.indexOf('jsdom') > -1)) { + storiesOf('Core|Errors', module) + .add('story throws exception', () => { + throw new Error('error'); + }) + // Story does not return something react can render + .add('story errors', () => null); +} From a42250f24df2e41aead99f4f43c04675891b1550 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 13 Aug 2018 10:23:07 +1000 Subject: [PATCH 5/5] Also skip stories on Chromatic We would probably need to render these stories inside the manager UI for this to make sense in Chromatic (which detects errors) --- examples/official-storybook/stories/core.stories.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/examples/official-storybook/stories/core.stories.js b/examples/official-storybook/stories/core.stories.js index 4b372a00f0c3..35088648b742 100644 --- a/examples/official-storybook/stories/core.stories.js +++ b/examples/official-storybook/stories/core.stories.js @@ -35,7 +35,12 @@ storiesOf('Core|Events', module).add('Force re-render', () => ( // Skip these stories in storyshots, they will throw -- NOTE: would rather do this // via a params API, see https://github.com/storybooks/storybook/pull/3967#issuecomment-411616023 -if (navigator && navigator.userAgent && !(navigator.userAgent.indexOf('jsdom') > -1)) { +if ( + navigator && + navigator.userAgent && + !(navigator.userAgent.indexOf('jsdom') > -1) && + !(navigator.userAgent.indexOf('Chromatic') > -1) +) { storiesOf('Core|Errors', module) .add('story throws exception', () => { throw new Error('error');