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

Fixed GitHub dependencies #520

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixed GitHub dependencies #520

wants to merge 2 commits into from

Conversation

jduncanator
Copy link

Optimized the process of installing dependencies from GitHub. It now uses the tarball of the CURRENT commit in master. Previously it was cloning the entire repository which wasn't needed and when it comes to express and socket.io, things get pretty big. This also fixes some issues that npm on Windows and other OSes have with malformed git addresses.

Optimized the process of installing dependencies from GitHub. It now
uses the tarball of the CURRENT commit in master. Previously it was
cloning the entire repository which wasn't needed and when it comes to
express and socket.io, things get pretty big. This also fixes some
issues that `npm` on Windows and other OSes have with malformed git
addresses.
@ahumellihuk
Copy link

This one fixes the issue. Good job!

@jduncanator
Copy link
Author

@popcorn-time Can we get this merged? I can't do anything at the moment unless I manually edit the packages.json file so I can pull in dependencies.

@Patineta Patineta added this to the Beta 3 milestone Mar 13, 2014
@Patineta Patineta modified the milestone: Beta 3 Mar 13, 2014
@simofacc
Copy link

I think we also need to fix github URLs which I believe should be in the following format

"adm-zip" : "git+https://github.com/gpt-modules/adm-zip.git",

instead of just "adm-zip" : "https://github.com/gpt-modules/adm-zip/tarball/master", otherwise I believe npm install would not work correctly and show errors.

@jduncanator
Copy link
Author

@simofacc No. You misunderstand how my pull request works Under normal circumstances you would be correct, however npm's Git (and GitHub) support is lousy at best and screws up under most platforms. It also requires the user to have Git installed and in their PATH.

The method I have used actually uses npm's tarball install feature that will grab the tarball from GitHub, extract it, and install it from the tarball. This way, we aren't cloning an entire repository recursively, it removes the dependency for Git and also has better support across platform.

@simofacc
Copy link

Ok that is good then.

@jduncanator
Copy link
Author

@simofacc Yea. I would have much rathered to have used the Git URLs however on Windows I just could not for the life of me get it to work. Browsing over the NPM source code it actually looks like using the GitHub feature of NPM (eg. "package": "github-user/repo") actually internally grabs the master tarball anyway (however that GitHub feature is even more broken than the Git one)!

@Sekhmet
Copy link

Sekhmet commented Mar 14, 2014

@jduncanator You should use git+https://, not https://

@jduncanator
Copy link
Author

@Sekhmet Did you even read my above comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants