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

Avoid logging an object on compilation errors #2121

Closed
wants to merge 4 commits into from

Conversation

derekstavis
Copy link
Contributor

@derekstavis derekstavis commented Oct 24, 2017

Issue

In certain configurations Storybook might log the whole Webpack
build object when warnings or errors are thrown. This was caused
by a logger.error call inside the index.js catch.

What I did

This fixes by logging the error only when it's an instance of an error.

How to test

This branch was tested on https://github.com/pagarme/pilot. Before
this changes, if an error happens while compiling (e.g. imported file not
found) a big object is logged:

screenshot from 2017-10-24 11-31-24

Resolves
#1889.

Is this testable with jest or storyshots?

Nope.

Does this need a new example in the kitchen sink apps?

Nope.

Does this need an update to the documentation?

Nope.

If your answer is yes to any of these, please make sure to include it in your PR.

In certain configurations Storybook might log the whole Webpack
build object when warnings or errors are thrown. This was caused
by a `logger.error` call inside the `index.js` catch. This fixes by logging
the error only when it's an instance of an error.
@codecov
Copy link

codecov bot commented Oct 24, 2017

Codecov Report

Merging #2121 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2121      +/-   ##
==========================================
- Coverage   21.42%   21.42%   -0.01%     
==========================================
  Files         263      263              
  Lines        5805     5806       +1     
  Branches      703      703              
==========================================
  Hits         1244     1244              
+ Misses       4012     4003       -9     
- Partials      549      559      +10
Impacted Files Coverage Δ
app/react/src/server/index.js 0% <0%> (ø) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/vue/src/server/babel_config.js 0% <0%> (-77.42%) ⬇️
addons/knobs/src/KnobManager.js 32% <0%> (ø) ⬆️
addons/info/src/components/Props.js 36.36% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/layout.js 12.5% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.63% <0%> (ø) ⬆️
... and 21 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 7f5c890...ca27409. 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.

Hi @derekstavis, Thank you for this!

Could you apply the same fix for app/vue and app/react-native?

@danielduan
Copy link
Member

@derekstavis would you like help putting this into the other 2 apps?

@derekstavis
Copy link
Contributor Author

Yes, will do sometime today! Thanks!

@danielduan
Copy link
Member

@derekstavis we're looking to get a new release out sometime today. would love to have this in.

@Hypnosphi
Copy link
Member

Opened #2199 instead

@Hypnosphi Hypnosphi closed this Oct 31, 2017
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.

4 participants