This repository has been archived by the owner. It is now read-only.

Always update git urls #4104

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@robertkowalski
Copy link
Member

robertkowalski commented Nov 7, 2013

Do directly an update if a git url is specified in the package.json
For outdated, print wanted=git and latest=git

Additionally, fixing a small, still unknown bug in update,
regarding the positions of where, latest and req, introduced
by 2f7fd62

Fixes #1727

@luk-

This comment has been minimized.

Copy link
Contributor

luk- commented Nov 7, 2013

Can you make installing have the same behavior?

@robertkowalski

This comment has been minimized.

Copy link
Member

robertkowalski commented Nov 7, 2013

You mean that doing

npm install $GIT_REPO_URL # first time
npm install $GIT_REPO_URL # second time

will update on the second time?

@robertkowalski

This comment has been minimized.

Copy link
Member

robertkowalski commented Nov 8, 2013

@luk- I was able to reproduce the same issue for installing and updated the PR.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Nov 9, 2013

LGTM, let's get this in :D

@luk-

This comment has been minimized.

Copy link
Contributor

luk- commented Nov 9, 2013

lgtm

Always update git urls
Do directly an update if a git url is specified in the package.json
For outdated, print wanted=git and latest=git

Additionally, fixing a small, still unknown  bug in update,
regarding the positions of where, latest and req, introduced
by 2f7fd62

Fixes #1727
@robertkowalski

This comment has been minimized.

Copy link
Member

robertkowalski commented Nov 15, 2013

@luk- @domenic I had a second iteration on that one:

  • as #4107 got merged I changed the test to use the common helper
  • I changed the implementation in install.js to a less invasive one, not changing the whole targetin the beginning.

Please have a second look at it, and sorry for any inconvenience.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Nov 19, 2013

So I think this is better than the status quo, and still give it a :shipit:. But, if we were to merge this, I'd want to open a new issue, "don't update git repos unless they are out of date." Because the ideal behavior is something like:

  • If package.json points to a commit hash and the local copy is already at that commit hash, do no HTTP requests.
  • If package.json points to a tag, go fetch the metadata for that tag from the repo, and check if the commit hash matches the local one. If so, do a full tarball download and unpack; otherwise, do no further HTTP.
  • If package.json points to no tag (i.e. master), go fetch the metadata for master, and proceed as in the previous case.

I'm not sure how much of this is possible given what information and technology we have available. (How do you tell what commit the local copy is on? What does 'getting metadata" mean? Are there easy ways to do this stuff for GitHub, but not for arbitrary Git servers?) I would bet, though, that Git-based package managers like Bower or Volo have already figured this out.

Probably something to tackle after install.js/cache.js gets some love. Anyway, yeah, ship this :). It is a nice unobtrusive change with good tests.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Nov 26, 2013

Follow-up bug filed as #4191, so since this was merged in 5829ecf we can close this :D

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