Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Don't run prepublish on npm install --production. #4461

Closed
wants to merge 1 commit into from

6 participants

Tim Oxley Domenic Denicola Alex Kocharin Robert Kowalski isaacs Forrest L Norvell
Tim Oxley
Collaborator
  • npm install usually installs devDependencies+regular deps
  • npm install --production only installs non-devDependencies
  • npm install always runs prepublish scripts
  • prepublish scripts often rely on dev dependencies
  • Problem: npm install --production will try invoke missing devDependencies

This pull request prevents prepublish scripts from running when the --production flag is present.

Seems to make sense to me.

Although…
People tend to flip out when things change though so it may not be worth rocking the boat.

I wonder if "breaking" behaviour changes such as this should obey semver conventions, you could consider npm's command line it's api i.e. when will npm hit 2.x?

test/tap/install-prepublish/package.json
@@ -0,0 +1,4 @@
+{ "name":"install-prepublish",
+ "version":"1.2.5",
+ "scripts": {"prepublish":"node -e 'process.kill(process.pid,\"SIGSEGV\")'"}}
Domenic Denicola Collaborator
domenic added a note

Maybe just log something the console instead of using a Unix-only thing? I know you copied this from another test, but that test was specifically about lifecycle signals.

Tim Oxley Collaborator

sure thing, that makes more sense.

Tim Oxley Collaborator

Fixed, rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Tim Oxley timoxley Don't run prepublish on npm install --production.
* npm install usually installs dev+regular deps
* npm install --production only installs dev deps
* npm install always runs prepublish
* prepublish scripts often rely on dev dependencies
* npm install --production will try invoke missing dependencies
de8f345
Domenic Denicola
Collaborator

LGTM, would like another maintainer to sign off first, e.g. @robertkowalski

Alex Kocharin

I'd say that this behaviour simply doesn't make sense. All right, I understand the need of 'prepublish' in dev mode (although it's a WAT from the first glance, and multiple issues confirm that). But when people accept that and learn to live with it, they will discover than in production mode it doesn't in fact happen. For this simple feature two WTFs is just too much.

I wonder if "breaking" behaviour changes such as this should obey semver conventions, you could consider npm's command line it's api i.e. when will npm hit 2.x?

Nobody cares about semver anyway, look how npm@1.3.19 broke stuff that supposed to be at least minor. :)

Robert Kowalski
Collaborator

Uhm, really unsure about this PR. I like it, but this will break some weird deployment configurations I think, especially for people that do not publish their packages first and e.g. install them from git repositories during deployment?

Tim Oxley
Collaborator

@robertkowalski that's a point, is there a current strategy for introducing potentially breaking changes in npm? Perhaps such a change (and any other npm behaviour changes) only lived in npm on the unstable branch of node? Might be a good pattern to adopt so people get a chance to upgrade and npm has a diplomatic strategy for evolving beyond breaking changes.

Domenic Denicola
Collaborator

Sounds like we'll need @isaacs to make a decision here.

jongleberry jonathanong referenced this pull request in reworkcss/rework
Closed

Travis builds failing on make command #149

isaacs
Owner

I think there's good intention here, and I was kinda sold on it at first, but it's just too eyebrow-raising. Having --production do different stuff entirely seems overly weird.

isaacs isaacs closed this
Forrest L Norvell othiym23 added the lifecycle label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 10, 2014
  1. Tim Oxley

    Don't run prepublish on npm install --production.

    timoxley authored
    * npm install usually installs dev+regular deps
    * npm install --production only installs dev deps
    * npm install always runs prepublish
    * prepublish scripts often rely on dev dependencies
    * npm install --production will try invoke missing dependencies
This page is out of date. Refresh to see the latest.
1  lib/install.js
View
@@ -154,6 +154,7 @@ function install (args, cb_) {
return target
}), where, context, function(er, results) {
if (er) return cb(er, results)
+ if (!opt.dev) return cb(null, results)
lifecycle(data, "prepublish", where, function(er) {
return cb(er, results)
})
27 test/tap/install-prepublish.js
View
@@ -0,0 +1,27 @@
+var test = require("tap").test
+var npm = require.resolve("../../bin/npm-cli.js")
+var node = process.execPath
+var exec = require("child_process").exec
+var path = require("path")
+var pkg = path.resolve(__dirname, "install-prepublish")
+
+test("prepublish runs on install", function (t) {
+ t.plan(2)
+ exec([node, npm, "install"].join(" "), {
+ cwd: pkg
+ }, function(err, stdout) {
+ t.ifError(err)
+ t.ok(stdout.match(/prepublish ran\./))
+ })
+})
+
+test("prepublish does not run on install --production", function (t) {
+ t.plan(2)
+ exec([node, npm, "install", "--production"].join(" "), {
+ cwd: pkg
+ }, function(err, stdout) {
+ t.ifError(err)
+ t.ok(!stdout.match(/prepublish ran\./))
+ })
+})
+
4 test/tap/install-prepublish/package.json
View
@@ -0,0 +1,4 @@
+{ "name":"install-prepublish",
+ "version":"1.2.5",
+ "scripts": {"prepublish":"node -e 'console.log(\"prepublish ran.\")'"}}
+
Something went wrong with that request. Please try again.