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

Changed isValidLicense Check #6134

Merged
merged 6 commits into from
Aug 22, 2018
Merged

Conversation

irrationalRock
Copy link
Contributor

@irrationalRock irrationalRock commented Jul 21, 2018

Summary
This PR fixes #6009.

It no longer throws an error when given " ".

Motivation

Yarn would not allow you to install a package if it had " " in it.

Test plan
When running yarn add mongoose-post-find, it now runs without producing an error.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

I think you're on the right track with using a try...catch block around validateLicense, but there are a few problems here still.

The original call to validateLicense is still there, and would still throw an exception when called. Instead of repeating that call, the result of the one inside the try...catch should be stored in a variable. Then you can reference that variable below when checking validForNewPackages.

The catch block also shouldn't be empty. If the validateLicense function throws an exception, it likely means the license isn't valid. That should return false, so that the warning message appears.

@Gudahtt
Copy link
Member

Gudahtt commented Aug 6, 2018

Also, this bug was also just fixed in version 3.0.4 of validate-npm-package-license. Updating to that version instead would fix the problem as well.

@irrationalRock
Copy link
Contributor Author

Thanks for the advice. I updated the package and it no longer throws the error.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Could you remove these changes in src/util/normalize-manifest/util.js now? They should no longer be needed.

@Gudahtt
Copy link
Member

Gudahtt commented Aug 9, 2018

Thanks! The last thing that comes to mind is that you updated the yarn.lock file to point at 3.0.4, but not the package.json file. That should be updated as well, to ensure it stays above or at that minimum version.

@irrationalRock
Copy link
Contributor Author

Thanks for the help. I updated the yarn.lock and package.json file.

package.json Outdated
@@ -46,7 +46,7 @@
"tar-stream": "^1.6.1",
"uuid": "^3.0.1",
"v8-compile-cache": "^2.0.0",
"validate-npm-package-license": "^3.0.3",
"validate-npm-package-license": "3.0.4",
Copy link
Member

@Gudahtt Gudahtt Aug 10, 2018

Choose a reason for hiding this comment

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

That ^ should stay there. We don't want to pin this dependency and prevent future updates.

@irrationalRock
Copy link
Contributor Author

Thanks for the suggestion. I changed it to ^.

@arcanis
Copy link
Member

arcanis commented Aug 22, 2018

Thanks 💯

@arcanis arcanis merged commit d63856d into yarnpkg:master Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to install the mongoose-post-find package
3 participants