npm install inexplicably runs npm run prepublish in npm 1.2.0

[dfellis]

damocles@moya:~/oss/sloppy-queue-flow(master)$ npm install

> sloppy-queue-flow@0.1.10 prepublish /Users/damocles/oss/sloppy-queue-flow
> npm test && docco ./lib/sloppy-queue-flow.js && uglifyjs ./lib/sloppy-queue-flow.js > ./lib/sloppy-queue-flow.min.js && git commit -am "Automatic doc and minification for version $npm_package_version" && git tag $npm_package_version && git push && git push --tags


> sloppy-queue-flow@0.1.10 test /Users/damocles/oss/sloppy-queue-flow
> nodeunit ./test/test.js


test.js
✔ sloppyType
✔ sloppy
✔ badHandler
✔ kill
✔ processNextTickPatch
✔ jscoverage

OK: 13 assertions (1044ms)
docco: ./lib/sloppy-queue-flow.js -> docs/sloppy-queue-flow.html
# On branch master
nothing to commit (working directory clean)
npm ERR! sloppy-queue-flow@0.1.10 prepublish: `npm test && docco ./lib/sloppy-queue-flow.js && uglifyjs ./lib/sloppy-queue-flow.js > ./lib/sloppy-queue-flow.min.js && git commit -am "Automatic doc and minification for version $npm_package_version" && git tag $npm_package_version && git push && git push --tags`
npm ERR! `sh "-c" "npm test && docco ./lib/sloppy-queue-flow.js && uglifyjs ./lib/sloppy-queue-flow.js > ./lib/sloppy-queue-flow.min.js && git commit -am \"Automatic doc and minification for version $npm_package_version\" && git tag $npm_package_version && git push && git push --tags"` failed with 1
npm ERR! 
npm ERR! Failed at the sloppy-queue-flow@0.1.10 prepublish script.
npm ERR! This is most likely a problem with the sloppy-queue-flow package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm test && docco ./lib/sloppy-queue-flow.js && uglifyjs ./lib/sloppy-queue-flow.js > ./lib/sloppy-queue-flow.min.js && git commit -am "Automatic doc and minification for version $npm_package_version" && git tag $npm_package_version && git push && git push --tags
npm ERR! You can get their info via:
npm ERR!     npm owner ls sloppy-queue-flow
npm ERR! There is likely additional logging output above.

npm ERR! System Darwin 12.2.0
npm ERR! command "node" "/usr/local/bin/npm" "install"
npm ERR! cwd /Users/damocles/oss/sloppy-queue-flow
npm ERR! node -v v0.8.17
npm ERR! npm -v 1.2.0
npm ERR! code ELIFECYCLE
npm ERR! 
npm ERR! Additional logging details can be found in:
npm ERR!     /Users/damocles/oss/sloppy-queue-flow/npm-debug.log
npm ERR! not ok code 0

Hopefully this is something minor (accidentally replaced preinstall with prepublish somewhere)

[mfncooper]

Running prepublish as part of npm install is as designed, per Isaac.

[dfellis]

I refuse to believe that. prepublish is a trigger that clearly means "before publishing to NPM".

How am I supposed to do things like auto-test, generate documentation, and create git tags before publishing to NPM if Travis-CI and every single user of the library would end up running that same code!?

preinstall is the trigger before npm install and prepublish is the trigger before npm publish. Anything else is insanity.

[dfellis]

Okay, so I found the commit that's causing the issue, and I think there's a better way to solve the problem rather than cause inexplicable behavior to help some people out (and completely wreck all of my NPM modules).

What you wanted was to be able to run the prepublish script inside of a local in-directory install only, so this hack was added in for that because there's no way for the preinstall script to know that it was invoked that way. (This totally breaks my libraries because npm install is also used to bootstrap a testing environment for them on Travis-CI, and I use prepublish to actually take care of chores that need to be done prior to publishing, such as generating a minified version for browsers, building documentation from the source code, making the commit for these things and tagging that commit with the new version number.)

A better solution would be to expose an $npm_invocation environment variable (or similar) that passes down the string the end-user typed into their console, and then let the relevant script decide based on that what it should do.

For this particular use-case, preinstall.sh would look like this:

#!/usr/bin/env bash

if ["$npm_invocation" == "npm install"]; then
    ./prepublish.sh
fi

This, I think, is much more useful because ./prepublish.sh could be replaced with ./preupdate.sh or ./pretest.sh, or the script could crank up C++ compiler optimization if it sees --production.

This is all necessary because the $npm_lifecycle_event can't be used because it's still reporting prepublish rather than preinstall.

I'm willing to make a PR to add the $npm_invocation functionality and then I can simply disable this bizarre behavior if you're unwilling to remove it, but I need a way to turn off prepublish running during preinstall.

[isaacs]

This change was by design, and was carefully considered.

I would say you're doing way too much in your prepublish script. I suggest adding a Makefile with a publish: target that does all this stuff, and then runs npm publish at the end of it. Then you'd run make publish instead of npm publish.

Or, if that's not to your liking, you can set a config var on your local machine like npm config set do_my_fancy_prepublish_junk true and check the value $npm_config_do_my_fancy_prepublish_script in the environment in your prepublish script.

Or yet another workaround, you can have a script with a different name entirely like "mypublish", and run it with npm run mypublish. (It can end with npm publish, even.)

Far and away, the most common use case for prepublish is to compile coffeescript into javascript (or minify javascript into ugly javascript, which is essentially the same sort of task). Since this is deterministic, and not platform-dependent, it makes no sense to do it multiple times, on each install target in the preinstall event. It should be done, instead, right before publishing the package, so that it can be bundled in the arball. However, the objection to moving it there is that it no longer compiles CoffeeScript to JavaScript in npm install <noargs>. So, now it does.

Eventually, arbitrary pre/post/install scripts will be removed, and node-gyp will be the only thing that will be run at install time. Compilation of binary addons is the only justifiable use of install-time scripts.

It looks like these packages are going to be affected by this. On the other hand, these packages will benefit from the change.

[dfellis]

Your two links aren't working for me, though I can see what you're going for (it won't catch scripts that actually delegate to a sh file and do their magic in there because it was too long to keep as a JSON string).

Why wouldn't it make more sense to keep the semantic meaning of pre<something> intact and for projects that want special behavior when installed without arguments, have them detect that condition? You say it was carefully considered but I don't understand the logic at all.

This new behavior breaks the contract of the scripts interface, and if they're deprecated for gyp, I think it would be nice if NPM informed me with a console warning so I'd know to migrate my code off of it.

I just don't see this as doing "too much" in my prepublish script -- it's simply doing the housekeeping on the repo and making sure that I don't publish a release without minified code, or generated docs, or a missing git tag -- little details that such a script is perfect for.

[isaacs]

Ah, yes, markdown mangled the links. Fixed.

Why wouldn't it make more sense to keep the semantic meaning of presomething intact and for projects that want special behavior when installed without arguments, have them detect that condition?

Because most modules with a prepublish script want to have it do the exact same thing in both cases. So that's the default. I understand that it's not what you'd prefer. I'm sorry. You are the edge case in this scenario.

There are already several ways that you can work around this (listed in my previous message), and we're talking about fewer than a dozen packages. I'd even be happy to send you pull requests to change them.

[dfellis]

I've started migrating my code as suggested to a custom npm script. I still think this behavior is unexpected because prepublish, to me, is something only the author of the lib should run, coupled with the regularity npm <foo> runs the pre<foo> hook first, then if successful does its expected operation, and then if that's successful, runs the post<foo> hook.

You don't need to work on any PRs for me, but the other two authors in that list probably deserve a warning that the next time they bootstrap their dev environment for their libraries it will fail.

I guess I'm mostly upset because I've touted NPM as an example of a well-done source package manager that does exactly what you expect of it, and this just doesn't make much sense, even if it is more convenient for the majority of lib authors.

[Munter]

I'm with @dfellis
It defies all intuition about this hook.

And for the record I have been using the same hook exactly as @dfellis has, to make sure my unit tests pass before publishing, to to git housekeeping, to publish to the gazillion frontend module package managers etc.

I have no problem writing Makefiles, but really I prefer pre- and post-hooks to be intuitive.

[millermedeiros]

Having the same issue, I guess I'm always the edge case... Should add a new hook, don't break consistency.

[grncdr]

Far and away, the most common use case for prepublish is to compile coffeescript into javascript (or minify javascript into ugly javascript, which is essentially the same sort of task). Since this is deterministic, and not platform-dependent, it makes no sense to do it multiple times, on each install target in the preinstall event. It should be done, instead, right before publishing the package, so that it can be bundled in the arball.

This all makes sense, and worked well before (and after) this change.

However, the objection to moving it there is that it no longer compiles CoffeeScript to JavaScript in npm install . So, now it does.

This argument seems pretty weak to me, but maybe I don't understand it fully. Is there further discussion somewhere else that I missed?

My understanding is that the main (and probably only) reason for running npm install <no args> is to install dependencies and otherwise set up a package that has just been pulled down from source control. There's roughly two groups that would be doing this:

1. Developers - if you are hacking on a project, you will need to understand any build toolchain it uses anyways.
2. Automated systems (e.g. continuous integration service) - All of these support specifying the command(s) that they need to run after retrieving sources, so having them automatically compile sources is trivial. Furthermore, anybody using one of them already does this.

FWIW, I found this issue after the change broke my travis-ci builds. I don't mind modifying my Makefile to work around it, but I really don't understand who the user group is that needs npm install <noargs> to compile coffeescript etc.

EDIT: found this https://github.com/isaacs/npm/pull/3053 in which a use case for having scripts automatically run on plain npm install is outlined.

[isaacs]

The eventual goal for package.json scripts will be to run pre/post install ONLY on npm install <noargs> in the package dir, and just run node-gyp rebuild on install if there's a gyp file. There are steps to getting there. First it's deprecated, and an alternative path is provided for people who want to compile coffeescript and do other silly things to do their silly things in a way that doesn't depend on us all doing the same deterministic silly things on our computers as well.

[strk]

Does anyone have a pointer of where was this carefully considered ? A mailing list thread, another ticket or something ?

[grncdr]

Does anyone have a pointer of where was this carefully considered ? A mailing list thread, another ticket or something ?

None that I was able to find, (and I did a lot of searching when this issue first broke my CI setup). The justifications given in this issue are terrible as well. In particular...

Eventually, arbitrary pre/post/install scripts will be removed, and node-gyp will be the only thing that will be run at install time. Compilation of binary addons is the only justifiable use of install-time scripts.

... and ...

The eventual goal for package.json scripts will be to run pre/post install ONLY on npm install in the package dir, and just run node-gyp rebuild on install if there's a gyp file.

... are both well reasoned arguments against preinstall/postinstall, but have nothing to do with prepublish. The comments in this discussion (again, the only recorded discussion of the issue afaik) are:

• prepublish is a trigger that clearly means "before publishing to NPM".
• It defies all intuition about this hook.
• Should add a new hook, don't break consistency.
• Insanity.

This change was a clearly confusing and undesirable to many of the users of the "prepublish" hook, and it concerns me that @isaacs (who has absolute authority over a lot of critical infrastructure for node) would defend it.

[aseemk]

I'm upgrading to npm 1.2, and this just bit me too. I was pretty baffled as well.

The install-through-git use case in #3053 is interesting food for thought, though I'm pretty surprised that that's not being called an "edge case", and wanting to do things purely pre-publish is.

[jsdevel]

+1 @dfellis Prepublish is a nice way for me to sanity check my modules before sending them off to npm. To say that install should trigger publish is counter-intuitive.

I will resort to @isaacs hack for now, I.E. exporting a variable within my prepublish script, but it sure feels like bad design.

npm publish # runs prepublish script
npm install # runs (pre|post)install

[JamesMGreene]

@garthk:
Keep in that @iarna works for @npm, Inc... so, if anyone were going to be attuned to the NPM internals [and any forthcoming changes that would break the "in-publish" module's analysis technique], she is a prime candidate!

[garthk]

@JamesMGreene, perhaps that attunement is reflected in her README (emphasis mine):

This detects that its running as a part of publish command in a terrible, terrible way. NPM dumps out its config object blindly into the environment prior to running commands. This includes the command line it was invoked with. This module determines if its being run as a result of publish by looking at that env var. This is not a part of the documented npm interface and so it is not guarenteed to be stable.

[iarna]

Indeed, it's a cow path that we can pave, if that pattern is effective for people. As far as breaking in-publish goes, it probably is unlikely as I doubt we can change the dumping of the config object without breaking any number of packages. It's unfortunate that it's so unconstrained, but it's the code we all have to live with.

If we were to pave this cow path then yes, I'd want a more explicit env var. The whole prepublish lifecycle thing is something that we've discussed internally, but we haven't reached any conclusions (and likely wouldn't without further public discussion) and isn't currently on any roadmaps.

[olalonde]

+1 this is totally unexpected behaviour. Got bitten by this this evening.

[flockonus]

+1 prepublish is really surprising in a bad way - altho i can't complain about the docs: https://docs.npmjs.com/misc/scripts

Wanted to refer back to @isaacs comment above about deprecating this behaviour, a long time has passed

[lxe]

This also causes npm prepublish to get stuck in infinite loops:

"prepublish": "npm install && npm run build"

[david0178418]

Just another vote for the priority to be raised on this. I keep forgetting that "prepublish" doesn't actually mean "prepublish".

[mtscout6]

Is this not getting prioritized because it's closed?

[david0178418]

@mtscout6 Good question. I'm hoping this is being tracked in another issue. If an issue's been logged and it's agreed that something should happen at some point to fix it, it should be tracked somewhere. Otherwise, these types of things tend to fall off the radar.

[olalonde]

@david0178418 I just created a new issue here: #8402 , let's hope this one doesn't get closed.

[monolithed]

+1. cause problems for me