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

Ignore home package.json no license field #3531

Merged
merged 3 commits into from
May 5, 2018

Conversation

Keraito
Copy link
Contributor

@Keraito Keraito commented May 4, 2018

Issue: Fixes #3446

If the /home/user folder contained a package.json without a license field, the installation of getstorybook is cancelled. This warning is thrown regardless of anything in the current folder that you're running getstorybook in.

Reproduction would be to create a package.json without a license field in your home directory, initialize a project that would normally work with getstorybook, like through create-react-app, and then run getstorybook.

This is caused by the checking of the version of the generator dependencies which resulted in the warning being thrown and thus existing the installation at https://github.com/storybooks/storybook/blob/3539625d7f801a0fee1fb8c58f1ff57016c9513f/lib/cli/lib/latest_version.js#L34
(Thanks to @sunilhari for pointing this out).
In fact, if you for whatever reason have a package.json at your home directory without a license field (or anywhere along the path towards the current directory), the warning will be thrown at any yarn command. The result of actually running the command, however, is then outputted after the warning:
image

What I did

Using the knowledge from the screenshot above, I basically added an array of warning texts that will not stop the entire installation process.

This fix is entirely based on yarn and I did not check yet how this works with npm and if there are any differences. But I wanted to open this PR in order to see whether this solution is desired before spending more time on it.

@Hypnosphi
Copy link
Member

I did not check yet how this works with npm

Please do that

@Hypnosphi
Copy link
Member

Should we maybe just ignore "type": "warning"?

@Hypnosphi Hypnosphi added the patch:yes Bugfix & documentation PR that need to be picked to main branch label May 4, 2018
@Keraito
Copy link
Contributor Author

Keraito commented May 4, 2018

@Hypnosphi

Please do that

Running npm info babel-runtime version does not raise a warning for a missing license field in package.json in the home directory, so this is an yarn only issue as far as I can observe. Nonetheless, I also did a full getstorybook installation with npm, and all of it works.

While I did this, I noticed that the checking of the package versions is always done with yarn if it's installed (https://github.com/storybooks/storybook/blob/master/lib/cli/lib/latest_version.js#L4), and that the --use-npm flag is only pointed at the installation of dependencies. Is this intended? I'm not sure if old versions of yarn will cause issues with it, like what was seen with issues related to #3453.

Should we maybe just ignore "type": "warning"?

That would be easier yes, but is every warning from yarn/npm/both ones that can be ignored for the installation of storybook? Asking because I don't have sufficient knowledge of all specifics of both package manager to be able to answer that.

@Hypnosphi
Copy link
Member

Hypnosphi commented May 5, 2018

While I did this, I noticed that the checking of the package versions is always done with yarn if it's installed (https://github.com/storybooks/storybook/blob/master/lib/cli/lib/latest_version.js#L4), and that the --use-npm flag is only pointed at the installation of dependencies.

It would make sense to include --use-npm check to has_yarn helper (and probably rename it to use_yarn
https://github.com/storybooks/storybook/blob/master/lib/cli/lib/has_yarn.js

is every warning from yarn/npm/both ones that can be ignored for the installation of storybook?

In this particular helper we only want to catch the fact that a package isn't published. User will still see the warnings when the actual installation will happen.

@Keraito
Copy link
Contributor Author

Keraito commented May 5, 2018

In this particular helper we only want to catch the fact that a package isn't published. User will still see the warnings when the actual installation will happen.

Makes sense, will change it to ignore yarn warnings in general then.

It would make sense to include --use-npm check to has_yarn helper (and probably rename it to use_yarn

Does it make sense to put this change into another PR, or should I include it here?

@codecov
Copy link

codecov bot commented May 5, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3531      +/-   ##
=========================================
- Coverage   37.61%   37.6%   -0.01%     
=========================================
  Files         459     459              
  Lines       10377   10378       +1     
  Branches      928     927       -1     
=========================================
  Hits         3903    3903              
+ Misses       5932    5931       -1     
- Partials      542     544       +2
Impacted Files Coverage Δ
lib/cli/lib/latest_version.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/layout.js 20% <0%> (ø) ⬆️
app/react-native/src/server/addons.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/header.js 25.71% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/index.js 18.18% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.63% <0%> (ø) ⬆️
addons/storyshots/src/rn/loader.js 53.33% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
... and 75 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 3539625...b4ec73e. Read the comment docs.

@Hypnosphi
Copy link
Member

Does it make sense to put this change into another PR, or should I include it here?

I'm ok with both, please do whatever you're more comfortable with

reject(new Error(info.data));
}
});
if (info.type !== 'warning') {
Copy link
Member

Choose a reason for hiding this comment

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

info.type === 'error' would be more explicit

@Keraito
Copy link
Contributor Author

Keraito commented May 5, 2018

I'm ok with both, please do whatever you're more comfortable with

I will do it in another PR as at first glans it looks like the change will not be very trivial, isn't necessarily related to the original issue, and I don't want to block this patch cuz of it.

info.type === 'error' would be more explicit

Changed it

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.

2 participants