Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Allow for npm install from GitHub #156

Closed
neverfox opened this issue Jan 3, 2016 · 10 comments
Closed

Allow for npm install from GitHub #156

neverfox opened this issue Jan 3, 2016 · 10 comments

Comments

@neverfox
Copy link

neverfox commented Jan 3, 2016

I know this can be a contentious topic, but as the project is configured now, it's not possible to depend on the GitHub version of the library (for one, there's no build on install and, two, the files directive in package.json wouldn't provide the necessary files for one). At times like these, where major bug fixes are only available there until a new NPM release is published, this requires a fork with a committed build (as far as I know). I can live with that, but has anyone considered any acceptable reorganization of the project that makes a direct GitHub install possible?

@kimjoar
Copy link
Collaborator

kimjoar commented Jan 3, 2016

I don't know what bugfix you're thinking of, but if you need to depend on master, you can add the master branch as a dep in package.json, then after npm install you need to cd node_modules/redux-simple-router and npm install && npm run build. But beware that master is very different from 1.0.2.

@kimjoar kimjoar closed this as completed Jan 3, 2016
@neverfox
Copy link
Author

neverfox commented Jan 3, 2016

That's just it though. You cannot do that because of the files section of package.json. When you install from master, the only things that will end up in your node_modules are the two *.md files, LICENSE, and package.json; none of the source code. So the naive suggestion is to remove the files section from package.json so that I can do a post-install build. I imagine though that that will have some impact on your publication process because you don't want those files to go to npm. In that case, you could use an .npmignore file

But beware that master is very different from 1.0.2.

Yes, indeed, and I've already got my code refactored to work with the changes. It was a necessity because the double updates were killing my navigation.

@neverfox
Copy link
Author

neverfox commented Jan 3, 2016

I'm happy to do a PR if that sounds like an acceptable solution.

@kimjoar
Copy link
Collaborator

kimjoar commented Jan 3, 2016

Ah, so that's actually a con of #134 — didn't think about this use-case. I'll re-open and wait and see if others have any opinions on this. Hopefully we'll get a 2.0 out soon too.

@koba04
Copy link
Member

koba04 commented Jan 4, 2016

Indeed, how about adding src to the files?
I think it can cover this use case.

koba04@90ac7ce

% npm install https://github.com/koba04/redux-simple-router.git#add-src-to-files-fields
% cd node_modules/redux-simple-router
% npm install && npm run build
:
:
% tree -L 1
.
├── CHANGELOG.md
├── LICENSE
├── README.md
├── lib
├── node_modules
├── package.json
└── src

@neverfox
Copy link
Author

neverfox commented Jan 4, 2016

I guess the obvious question is "Why limit it in the first place?" To me, pulling from GitHub or the tarball should represent what you find in the repo itself. It's an odd situation to be in to find that they differ.

@koba04
Copy link
Member

koba04 commented Jan 4, 2016

I think npm install should only include files that are required to use it.
I think an opt-in approach with files fields is better than an opt-out approach with .npmignore, because managed files by a project can be increasing over time.

I guess npm install ${github.repo} is not the common use case.
In many other libraries, npm install ${github.repo} have different results from npm install ${library}` and its GitHub repository.

Additionally, in many cases, npm install ${github.repo} requires some custom scripts to use it.
In this case, I guess it should be used the forked repository that is added scripts for a build.

As a result, I think it is enough to add src to files fields.

koba04@90ac7ce

@neverfox
Copy link
Author

neverfox commented Jan 5, 2016

I think npm install should only include files that are required to use it.

From the registry sure, but when you are specifically requesting the master branch from GitHub, I think you should get...the master branch from GitHub. At that point, you are using NPM as a transport mechanism for something else.

And what about the tarball? Don't you think it should be an archive of the repo as it existed at a certain release, not just what you publish to NPM, which is really an unrelated concern?

@koba04
Copy link
Member

koba04 commented Jan 5, 2016

Indeed, I think the npm registry is not an archive site. It is a registry for npm as a package manager.
If I need an archive of the repository, I am usually using git clone from GitHub(master or releases).

@jlongster
Copy link
Member

I think @koba04 is right. For now let's just add src to the files field. Installing from npm and then manually running npm install inside the module works now.

I wish npm just ignored these fields when installing straight from a git repo. I would expect to get the full repo, but this works for now.

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

No branches or pull requests

4 participants