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

Fix isNpmUrl to handle http/https urls (Closes #9236) #9237

Closed
wants to merge 1 commit into from

Conversation

edi9999
Copy link
Contributor

@edi9999 edi9999 commented Oct 16, 2017

See #9236 for the details about the bug.

@apollo-cla
Copy link

@edi9999: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@edi9999 edi9999 force-pushed the fix-is-npm-url branch 2 times, most recently from 2fa5939 to eeb1d9d Compare October 16, 2017 12:57
Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR @edi9999! I've added a few small comments. Regarding the tests comment - isNpmUrl really should have had tests in place before you started working on this (sorry about that), so if you don't have time to wire them up, let us know. Thanks again!

History.md Outdated
@@ -4,6 +4,8 @@
[Issue #9196](https://github.com/meteor/meteor/issues/9196)
[PR #9198](https://github.com/meteor/meteor/pull/9198)

* Allow Npm.depends to specify any http or https URL [#9236](https://github.com/meteor/meteor/issues/9236)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the issue link to a new line, and prefix it with "Issue" (to line up with the other History.md entries).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -490,7 +490,7 @@ exports.isUrlWithSha = function (x) {
exports.isNpmUrl = function (x) {
// These are the various protocols that NPM supports, which we use to download NPM dependencies
// See https://docs.npmjs.com/files/package.json#git-urls-as-dependencies
return exports.isUrlWithSha(x) || /^(git|git\+ssh|git\+http|git\+https)?:\/\//.test(x);
return /^(git|git\+ssh|git\+http|git\+https|https|http)?:\/\//.test(x);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding some tests for isNpmUrl to verify the accepted dependency URL formats? They don't have to be that extensive, just one quick check for each accepted URL format, including http and https. The tests would go in tools/tests/utils-tests.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like to be sure that these new URLs are preserved correctly in npm-shrinkwrap.json, so that we don't keep trying to reinstall the packages when they've already been installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a few tests.

I would also like to be sure that these new URLs are preserved correctly in npm-shrinkwrap.json, so that we don't keep trying to reinstall the packages when they've already been installed.

Does this comment have to do with this MR ?

@benjamn
Copy link
Contributor

benjamn commented Nov 22, 2017

Merged to devel:

Thanks for fixing this @edi9999!

@benjamn benjamn closed this Nov 22, 2017
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.

None yet

4 participants