search

dthree / vorpal

Node's framework for interactive CLIs
http://vorpal.js.org
MIT License
5.64k stars 280 forks source link

chore: make in-publish module as a dev. dependency module #139

Closed dragosh closed 8 years ago

dragosh commented 8 years ago

Move in-publish node module into the dev dependency section.

scotthovestadt commented 8 years ago

@dthree @dragosh My understanding is that the prepublish hook is called when you install the module in any way and that dev deps are only installed when you run npm install vorpal and not when you install it from package.json, so I think it's necessary to have this in dependencies.

It's also an extremely small module with no dependencies of it's own so I am reluctant to move it unless someone can demonstrate it works in all cases as a dev dep. The documentation for this module shows installing it as a regular dep. There is a question about dev dep, but not answered: https://github.com/iarna/in-publish/issues/6

dthree commented 8 years ago

@scotthovestadt what was this added for?

scotthovestadt commented 8 years ago

@dthree Bug - https://github.com/dthree/vorpal/issues/128

For further background - https://github.com/npm/npm/issues/3059

TLDR - NPM calls prepublish hook after install and before publish. Very weird behavior. Common problem that this module solves.

dragosh commented 8 years ago

@scotthovestadt thanks for the info, and i agree that it is a weird npm behaviour https://github.com/npm/npm/issues/5154. Why not remove the prepublish hook and create a Makefile and trigger the publish via make publish, in the end only authorized user can publish the module not the end user. You can do a lot more instead of inline "commands in the package.json". You don't even need gulp anymore.

SHELL = bash
BIN = ./node_modules/.bin

# linting & testing
test:
    @$(BIN)/eslint lib/
    @$(BIN)/mocha --reporter spec
# publishing
# could be more advanced like publishing beta releases
publish: test
    @npm publish

.PHONY: test publish

this is just an example will not work in current setup, and don't forget to ignore the Makefile in the .npmignore file

PS: If you want i can create a PR with the Makefile plus adding test coverage on the unit tests.

scotthovestadt commented 8 years ago

@dragosh First of all, thank you for your thoughts. I appreciate your contribution to the discussion.

The reason I prefer to use Gulp and the NPM lifecycle for the build process is so that it's easy for other contributors to get involved. Creating a makefile would be unique to this project and someone not familiar with it (or that contributes to a lot of projects, like I do) may forget it.

Using an NPM pre-publish hook it's impossible to forget the publish hook. This module is only a couple lines, I don't personally agree it's a big issue.

Additionally, is the makefile solution cross-platform? I know that @dthree develops on a Windows machine.

dthree commented 8 years ago

Additionally, is the makefile solution cross-platform? I know that @dthree develops on a Windows machine.

Sad, but true.

@dragosh very appreciated, but I have to agree with @scotthovestadt on this one.

dragosh commented 8 years ago

sorry for the late reply, @scotthovestadt you're probably right. but keep in mind that this is a tool for prompt commands, adding extra 3rd party dependency to fix an npm issue is not a good idea especially if you don't lock the child dependencies with npm shrinkwrap

Worm publishes new versions to each of the owned modules with a “bugfix” level semver bump. This ensures the majority of dependent modules using the ^ or ~ signifier will include the self­replicating module during the next install. (https://www.kb.cert.org/vuls/id/319816)

I'm saying this because i'm building a CLI tool for a banking platform application based on the "power of vorpal" and i have to be careful with all the dependencies and their execution code, plus you don't want the in-publish module be the next left-pad :)

PS: i'm sure @dthree is using http://www.cygwin.com/ or similar, right :) ?

PPS: @dthree @scotthovestadt btw thanks for the hard work on making this amazing tool. i'll try to contribute as much as possible.

scotthovestadt commented 8 years ago

If it's consolation, the owner of in-publish works for NPM.

I agree the dependency tree needs to be locked down to individual versions.

On Monday, March 28, 2016, Dragos Oancea notifications@github.com wrote:

sorry for the late reply, @scotthovestadt https://github.com/scotthovestadt you're probably right. but keep in mind that this is a tool for prompt commands, adding extra 3rd party dependency to fix an npm issue is not a good idea especially if you don't lock the child dependencies with npm shrinkwrap ( https://www.kb.cert.org/vuls/id/319816) I'm saying this because i'm building a CLI tool for a banking platform application based on the "power of vorpal" and i have to be careful with all the dependencies and their execution code, plus you don't want the in-publish module be the next left-pad :)

PS: i'm sure @dthree https://github.com/dthree is using http://www.cygwin.com/ or similar, right :) ?

PPS: @dthree https://github.com/dthree @scotthovestadt https://github.com/scotthovestadt btw thanks for the hard work on makeing this amazing tool. i'll try to contribute as much as possible.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/dthree/vorpal/pull/139#issuecomment-202299239

dthree commented 8 years ago

Ugh. Welcome to the new norm of the Node world. I really don't want to have these conversations all of the time. One person does something, no mind the 3.8 billlion package downloads last month that weren't malicious.

Yes, we'll remove the ^s from our package.json.

@dragosh thank you for your kind words, and would love help on the project :smile: