Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add git+file:// URL protocol #3868

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

I added this to allow hosting private modules on a bare git repo, without needing to host that repo over HTTP or SSH.

I haven't run the full test suite yet as I'm behind a proxy which blocks git:// protocol, but other tests are passing.

Contributor

luk- commented Sep 7, 2013

What does this allow us to do that we cannot already do using npm i ~/path/to/module/folder which currently work?

It allows use of bare git repos - npm currently complains about not finding the package.json. It also allows specific tags to be referenced, for versioning.

Contributor

luk- commented Sep 7, 2013

It's complaining as it should be, in that case. Modules shouldn't be
installed if they don't have a package.json.

On Saturday, September 7, 2013, tehsenaus wrote:

It allows use of bare git repos - npm currently complains about not
finding the package.json. It also allows specific tags to be referenced,
for versioning.


Reply to this email directly or view it on GitHubhttps://github.com/isaacs/npm/pull/3868#issuecomment-23985877
.

Of course a bare git repo doesn't have a package.json... The contents of the repo on the other hand does. :)

Contributor

luk- commented Sep 7, 2013

Ok, fine. Please detail an example use.

On Saturday, September 7, 2013, tehsenaus wrote:

Of course a bare git repo doesn't have a package.json... The _contents_of the repo on the other hand does. :)


Reply to this email directly or view it on GitHubhttps://github.com/isaacs/npm/pull/3868#issuecomment-23994371
.

Deploying private modules via a git repo located on a shared drive. This mitigates the need for any central server to provide HTTP/SSH access.

Besides, file:/// is a protocol supported by git, I don't see why it shouldn't be supported by npm, especially when the implementation is such a small change!

Member

domenic commented Sep 28, 2013

This seems great to me; what convinced me was

Besides, file:/// is a protocol supported by git, I don't see why it shouldn't be supported by npm, especially when the implementation is such a small change!

However I cannot merge due to #3948; tagging as ready-to-merge.

Member

robertkowalski commented Nov 7, 2013

Seems I can test git related npm functionality soon in our test-suite! Great!

Member

robertkowalski commented Nov 7, 2013

(without doing a network call to external dependencies)

Contributor

luk- commented Nov 7, 2013

Something like this can only be merged if it only works on modules with
"private": true in their package.json.

On Thursday, November 7, 2013, Robert Kowalski wrote:

(without doing a network call to external dependencies)


Reply to this email directly or view it on GitHubhttps://github.com/isaacs/npm/pull/3868#issuecomment-28009100
.

Member

robertkowalski commented Nov 8, 2013

I totally understand your point @luk- - having a smarter feature would be really nice.

For me the userstory behind this feature is "I want to enable git from the local filesystem" and not "I want to install private marked repositories from the local filesystem". So for me the feature is a valid feature.

I already had an usecase for this, yesterday night for #4104 where I used a public repo on GitHub for testing the feature.

Contributor

luk- commented Nov 9, 2013

The reason I'm being difficult about this is because the feature's and its caveat are close enough to another request: supporting local filesystem modules in package.json dependencies (regardless of if they're in a repo), since you can already npm install path/to/module/. I discussed this with Isaac about 2 months ago, and his main requirement for supporting something like this would be to ensure that the modules are marked as private, something I agree with.

This requirement is to help prevent modules being published with what I would call an impossible dependency. A user can't install a module if one of its required dependencies is a reference to a nonexistent location on a local filesystem. I don't want to get into the nightmareish dependency situation in which people are publishing modules on npm that require users to have something installed on their system already — not any more than we already are in a few cases. You could make the argument that someone is currently able to include an invalid dependency, but I don't think that means we should add a feature that could bring forth havoc to thousands of users who expect their node modules to Just Work.

I guess you're looking at this from an agile point of view, which is fine if it works for you, but consider this userstory blocked by another userstory: modules cannot use dependencies on a filesystem path unless they are private modules.

Member

robertkowalski commented Nov 9, 2013

That is really reasonable. Thanks for the explanation.

robertkowalski added a commit to robertkowalski/npm that referenced this pull request Nov 9, 2013

Member

robertkowalski commented Nov 9, 2013

was quite easy to build, build this today.

robertkowalski added a commit to robertkowalski/npm that referenced this pull request Nov 16, 2013

robertkowalski added a commit to robertkowalski/npm that referenced this pull request Nov 21, 2013

Allow installations from git+file:// if the package is private
Throw error, in case they should be published
See #3868

robertkowalski added a commit to robertkowalski/npm that referenced this pull request Nov 22, 2013

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

@othiym23 othiym23 added review and removed ready-to-merge! labels Jul 23, 2014

@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

Member

robertkowalski commented Nov 15, 2014

this is possible now: #4108

sorry for the delay @tehsenaus

@othiym23 othiym23 removed the review label Nov 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment