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 error "storyshots is intended only to be used with storybook" f… #1441

Merged
merged 6 commits into from
Sep 13, 2017

Conversation

osdevisnot
Copy link

@osdevisnot osdevisnot commented Jul 9, 2017

…or transitive dependency on @storybook/react

Issue: When @storybook/react is not a direct dependency in package.json - the storyshots fails to execute tests with error "storyshots is intended only to be used with storybook". This is ok in normal circumstances, but limits wrapping "@storybook" into a higher order package.

For example: a project depends on a higher order package (maybe CLI tool). this higher order package brings in both @storybook/react and @storybook/addon-storyshots with appropriate config. The storyshots will throw above mentioned error.

What I did

Add a fallback check to see if package.json exists before bailing out on dependency being available.

How to test

Tested it by using this package in my project.

…or transitive dependency on @storybook/react
@codecov
Copy link

codecov bot commented Jul 9, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1441   +/-   ##
=======================================
  Coverage   21.32%   21.32%           
=======================================
  Files         257      257           
  Lines        5739     5739           
  Branches      686      679    -7     
=======================================
  Hits         1224     1224           
- Misses       3981     3993   +12     
+ Partials      534      522   -12
Impacted Files Coverage Δ
addons/storyshots/src/index.js 0% <ø> (ø) ⬆️
lib/ui/src/modules/ui/containers/left_panel.js 25.71% <0%> (ø) ⬆️
.../src/manager/containers/CommentsPanel/dataStore.js 34.97% <0%> (ø) ⬆️
addons/info/src/components/Node.js 39.43% <0%> (ø) ⬆️
...es__/update-addon-info/update-addon-info.output.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
app/react/src/client/preview/error_display.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/left_panel/header.js 29.62% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 8.51% <0%> (ø) ⬆️
... and 15 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 9f73119...80a769f. Read the comment docs.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@osdevisnot thanks for your contribution! One thing --
how do I test it on my machine?

@osdevisnot
Copy link
Author

osdevisnot commented Jul 10, 2017

Setup storyshots as you normally would, and then remove @storybook/react dependency from package.json but leave the installed version in node_modules.

Without this PR, you should be able to see above error.
@shilman

@shilman
Copy link
Member

shilman commented Jul 10, 2017

@osdevisnot OK great, thanks! I'll give it a whirl. FYI, "@storybook/react" in your initial comment was not escaped, so it just showed up as "" on github. I went through and fixed it for you, but for future reference ... 😉

@osdevisnot
Copy link
Author

Aw, I feel stupid.. will be more careful with markdown :( thanks for correcting it.

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 guess that makes sense!

@shilman
Copy link
Member

shilman commented Jul 22, 2017

@ndelangen looks like there are some CircleCI config issues when the pull is coming from a fork of our repo?

@ndelangen
Copy link
Member

Sorry for the delay, LGTM!

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.

None yet

4 participants