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

Improve yarn detection #3453

Merged
merged 2 commits into from
Apr 20, 2018
Merged

Conversation

Keraito
Copy link
Contributor

@Keraito Keraito commented Apr 19, 2018

Issue: Currently, getstorybook uses yarn when it's available on the system even in cases where npm is used in the current project, causing errors. (closes #3384)

What I did

I changed the condition for using yarn from checking yarn --version to looking for a yarn.lock file in the current project.

How to test

  • I ran the e2e tests and all of them passed. Testing this with a (additional) fixture seemed excessive based on the fixtures, but please correct me if I'm wrong.

@Keraito
Copy link
Contributor Author

Keraito commented Apr 19, 2018

After pushing I thought of the case where yarn.lock exists, but yarn is not installed on the system. The process will stop as no yarn is available, is that a (rare) case that is storybook's responsibility to cover?

@Hypnosphi
Copy link
Member

is that a (rare) case that is storybook's responsibility to cover?

I think it wouldn't hurt to have an additional check

@Hypnosphi Hypnosphi added bug cli patch:yes Bugfix & documentation PR that need to be picked to main branch labels Apr 19, 2018
@codecov
Copy link

codecov bot commented Apr 19, 2018

Codecov Report

Merging #3453 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3453      +/-   ##
==========================================
- Coverage   37.15%   37.14%   -0.02%     
==========================================
  Files         465      465              
  Lines       10316    10320       +4     
  Branches      925      919       -6     
==========================================
  Hits         3833     3833              
- Misses       5933     5963      +30     
+ Partials      550      524      -26
Impacted Files Coverage Δ
lib/cli/lib/has_yarn.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/RerunButton.js 0% <0%> (ø) ⬆️
...tive/src/preview/components/StoryListView/index.js 0% <0%> (ø) ⬆️
addons/storyshots/src/rn/loader.js 53.33% <0%> (ø) ⬆️
...i/src/modules/ui/components/stories_panel/index.js 20.63% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/menu_item.js 19.14% <0%> (ø) ⬆️
...tories_panel/stories_tree/tree_decorators_utils.js 45.23% <0%> (ø) ⬆️
addons/info/src/components/types/Shape.js 66.03% <0%> (ø) ⬆️
lib/core/src/server/utils.js 40.47% <0%> (ø) ⬆️
addons/info/src/components/types/OneOfType.js 60% <0%> (ø) ⬆️
... and 67 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 bc6c66d...161fcec. Read the comment docs.

@danielduan
Copy link
Member

I think we should default to using npm unless (yarn is installed AND (has yarn lock OR npm not installed))

We're bound to get issues with these edge cases given the number of users we have.

@Keraito
Copy link
Contributor Author

Keraito commented Apr 19, 2018

I changed the function to behave as specified in #3453 (comment), defaulting to npm unless the condition matches. All e2e tests still pass on my end.

@ndelangen
Copy link
Member

This looks mergable, what's wrong with the CI @Hypnosphi ?

@Hypnosphi
Copy link
Member

Connection timeout, I've restarted

@Hypnosphi Hypnosphi merged commit 4cd9edc into storybookjs:master Apr 20, 2018
@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 27, 2018
Hypnosphi added a commit that referenced this pull request Apr 27, 2018
@shilman shilman changed the title Change yarn detection Improve yarn detection Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cli patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants