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

Gracefully handle fatal webpack errors. #1918

Merged
merged 1 commit into from
Sep 30, 2017

Conversation

bradleyayers
Copy link
Contributor

Issue:

When a fatal webpack error occurs (e.g. wrong configuration) storybook's error handler incorrectly assumes that the stats object is provided by webpack. See https://webpack.js.org/api/node/#error-handling for details on error handling.

The effect is the callback throwing an exception at https://github.com/storybooks/storybook/blob/master/app/react/src/server/build.js#L86, with a message to the effect of 'undefined has no property hasErrors'.

This is a problem because some webpack loaders will run timers or open sockets that prevent the node process from exiting, and when the callback throws an error before reaching the process.exit(1); code which would kill it.

There have been cases where in continuous integration environments the build will run indefinitely (which is a problem when you pay per-second).

What I did

Added a simple check to ensure stats is passed, before using it.

How to test

Create a webpack configuration that causes a fatal error, and run storybook.

@codecov
Copy link

codecov bot commented Sep 30, 2017

Codecov Report

Merging #1918 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1918   +/-   ##
=======================================
  Coverage   21.31%   21.31%           
=======================================
  Files         257      257           
  Lines        5737     5737           
  Branches      691      687    -4     
=======================================
  Hits         1223     1223           
+ Misses       3989     3982    -7     
- Partials      525      532    +7
Impacted Files Coverage Δ
app/react/src/server/build.js 0% <ø> (ø) ⬆️
...ponents/left_panel/stories_tree/tree_decorators.js 33.33% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 50.47% <0%> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 12% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/menu_item.js 19.14% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/filters.js 47.36% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/down_panel.js 23.52% <0%> (ø) ⬆️
addons/info/src/components/markdown/code.js 24.13% <0%> (ø) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3468f58...aaea315. Read the comment docs.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

👌

@ndelangen
Copy link
Member

Thank you @bradleyayers !!!

@ndelangen ndelangen merged commit 928166c into storybookjs:master Sep 30, 2017
@Hypnosphi
Copy link
Member

@ndelangen do we neet the same for app/vue?

@ndelangen
Copy link
Member

Very good point! Yes we do.

@ndelangen ndelangen added the bug label Sep 30, 2017
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.

3 participants