Skip to content

Allow installations from git+file:// if the package is private #4108

Closed
wants to merge 1 commit into from

4 participants

@robertkowalski
npm member

See #3868

@robertkowalski
npm member

Question: Please note the different usage of the variable what in the both PR

Always update git urls - where I am preparing all git urls
https://github.com/robertkowalski/npm/blob/05d14e130483d69e6ef0bdbae90e5e509ee7e69f/lib/install.js#L153

VS

Allow installations from git+file:// if the package is private - where I use split("@").pop() on the unprepared string
https://github.com/robertkowalski/npm/blob/b9e3beed076add73f61bff05ca32b98d7db5ae55/lib/install.js#L667

As both are not merged yet, what do you think is the way to go?

@robertkowalski
npm member

Ok, figured the - in my opinion - best way out for that issue and changed #4104

Please review, I think this might close the long running #2442 with some luck

@luk-
luk- commented Nov 18, 2013

I really hate to be that guy here but I think we should probably revisit how we're adding things to install.js. This file is really big and is pretty hot code – and probably should be a collection of smaller modules so we can increase interoperability and hopefully reduce the risk of breaking stuff.

Also, it's possible I wasn't really clear when describing supporting local filesystem dependencies, but I don't think we need to get in the way of someone installing something, it's more on the publish side. There's really no benefit to the user or safety for the registry to do a sanity check for "private": true when someone is attempting to npm install in a directory with local filesystem dependencies. When I made a fuss about special requirements for these local private dependencies, my goal was to not have us land the feature without protection from potential publishing, not really as much to nag users during an install.

Lastly, does this work the same on Windows? Does it support directories as well? npm isn't intrinsically tied to git, so if we're adding support for local things as first-class dependencies, it should probably be more than just via a local git repo.

Anyway, sorry, you've been doing awesome work and I feel bad for bringing this up, but I think it has to be said.

@robertkowalski
npm member

Hey @luk-,

thanks for your feedback, I would really like to help splitting up install.js - but that would be another PR.

Please have a look at https://github.com/robertkowalski/npm/blob/4d0be594c1b9de2501f5544f53b5d8e441fa088b/lib/publish.js#L132 and the whole diff from publish.js - there is a check for preventing publishing modules. I can remove the check when installing modules, that's no problem for me. You are right that this probably makes no sense, but from the previous context I thought it might be worthy to include the check there too.

I don't get your last point regarding directories. What do you mean with directories?

What do I have to do to get this merged now, despite removing the check from install.js or how do we continue?

@robertkowalski
npm member

Ah, forgot to add: I definitely want this feature (local git dependencies) to be able to convert more of these tests https://github.com/isaacs/npm/blob/master/test/packages/npm-test-shrinkwrap/package.json#L7 to tap, without connecting to github or any other git repos over the network and to be able to test anything new git related

@robertkowalski robertkowalski Allow installations from git+file:// if the package is private
Throw error, in case packages are going to be published
See #3868
fbd5189
@robertkowalski
npm member

Had the time to test this on windows and work further on this issue:

It works on windows and I removed the nagging of users during install.

I like the idea with the local directories, but let's take this as a first step.

@domenic
npm member
domenic commented Nov 22, 2013

I am +1. It would be nice to get @isaacs to make a judgement call though.

@luk-
luk- commented Nov 22, 2013
@othiym23

I'll take another look at this as part of landing #5629. Thanks for your patience, @robertkowalski!

@othiym23 othiym23 added this to the 2.0.0 milestone Sep 2, 2014
@othiym23 othiym23 removed this from the 2.0.0 milestone Sep 13, 2014
@robertkowalski
npm member

hey @othiym23 - i would love to port that patch to the current codebase to catch up - if you still consider it to land. i can really say that i really was depending on this to write integrations tests that covered an install from git - and additionally the initial wish to add this came from the community. i am also happy to close this - no hard feelings

@othiym23

@robertkowalski: https://github.com/npm/npm/blob/master/test/tap/git-npmignore.js#L43 😁 This got merged in through the backdoor when we were making changes to npa, I think.

There may be bugs in how it works. File those as separate issues?

@othiym23 othiym23 closed this Nov 15, 2014
@othiym23 othiym23 removed the review label Nov 15, 2014
@robertkowalski
npm member

❤️

@robertkowalski
npm member

@othiym23 will close the issues related to it

@robertkowalski robertkowalski deleted the robertkowalski:imp/git-file branch Nov 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.