Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Commit

Permalink
publish: simplify lifecycle invocation
Browse files Browse the repository at this point in the history 
Previously (in 0ab0c11) support was
added for running the publish and postpublish lifecycle scripts. Some
work was done to ensure that this was run in the correct path both when
publishing from the package's working directory, as well as when
publishing from a package tarball file or URL. The latter case is
interesting, because it would only ever work for lifecycle scripts that
didn't rely on bins exported by (dev)dependencies, due to being run
directly from the cached package.json. In the process, there was a lot
of inline use of ternary operators that made it very hard to understand
what was going on.

There are two ways to fix this:

1. An expensive way, which is to explode the package once it's been
   cached, install it, and then run the lifecycle scripts from that
   directory.
2. A cheap way, which is to assume that whatever scripts needed to be
   run were already run when packing the tarball or remote package, and
   just don't run the lifecycle scripts at all unless you're publishing
   from the working directory.

This patch opts for approach #2. Publishing directly from tarballs is an
option for power users, and having lifecycle events fire in that case is
a conflation of the build and upload stages of publish that cuts against
what (I believe) most users are trying to do, which is to build an
artifact, certify it, and then only publish it if certification
succeeds.

However, this is technically a breaking change, and as such should
either ship before `npm@4` becomes latest, or be held for `npm@5`. (I'd
prefer the former, but leave that for the team to decide.)

Credit: @othiym23
Fixes: #14493
Reviewed-By: @iarna
PR-URL: #14502
  • Loading branch information
@othiym23
othiym23 committed Nov 3, 2016
1 parent 731ae52 commit 8b32d67
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 94 deletions.
2 changes: 1 addition & 1 deletion lib/build.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ function linkBins (pkg, folder, parent, gtop, cb) {
}
var binRoot = gtop ? npm.globalBin
: path.resolve(parent, '.bin')
log.verbose('link bins', [pkg.bin, binRoot, gtop])
log.verbose('linkBins', [pkg.bin, binRoot, gtop])

asyncMap(Object.keys(pkg.bin), function (b, cb) {
linkBin(
Expand Down
65 changes: 31 additions & 34 deletions lib/publish.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,39 +52,38 @@ function publish (args, isRetry, cb) {
if (!data.version) return cb(new Error('No version provided'))
}

// Error is OK. Could be publishing a URL or tarball, however, that means
// that we will not have automatically run the prepublish script, since
// that gets run when adding a folder to the cache.
if (er) return cacheAddPublish(arg, false, isRetry, cb)
else cacheAddPublish(arg, true, isRetry, cb)
})
}

// didPre in this case means that we already ran the prepublish script,
// and that the 'dir' is an actual directory, and not something silly
// like a tarball or name@version thing.
// That means that we can run publish/postpublish in the dir, rather than
// in the cache dir.
function cacheAddPublish (dir, didPre, isRetry, cb) {
npm.commands.cache.add(dir, null, null, false, function (er, data) {
if (er) return cb(er)
log.silly('publish', data)
var cachedir = path.resolve(cachedPackageRoot(data), 'package')
chain(
[
!didPre && [lifecycle, data, 'prepublish', cachedir],
[lifecycle, data, 'prepublishOnly', cachedir],
!didPre && [lifecycle, data, 'prepare', cachedir],
[publish_, dir, data, isRetry, cachedir],
[lifecycle, data, 'publish', didPre ? dir : cachedir],
[lifecycle, data, 'postpublish', didPre ? dir : cachedir]
],
cb
)
// if readJson errors, the argument might be a tarball or package URL
if (er) {
npm.commands.cache.add(arg, null, null, false, function (er, data) {
if (er) return cb(er)
log.silly('publish', data)
var cached = path.resolve(cachedPackageRoot(data), 'package') + '.tgz'
// *publish* lifecycle scripts aren't run when publishing a built artifact
// go to the next step directly
publish_(arg, data, isRetry, cached, cb)
})
} else {
var dir = arg
npm.commands.cache.add(dir, null, null, false, function (er, data) {
if (er) return cb(er)
log.silly('publish', data)
var cached = path.resolve(cachedPackageRoot(data), 'package') + '.tgz'
// `prepublish` and `prepare` are run by cache.add
chain(
[
[lifecycle, data, 'prepublishOnly', dir],
[publish_, dir, data, isRetry, cached],
[lifecycle, data, 'publish', dir],
[lifecycle, data, 'postpublish', dir]
],
cb
)
})
}
})
}

function publish_ (arg, data, isRetry, cachedir, cb) {
function publish_ (arg, data, isRetry, cached, cb) {
if (!data) return cb(new Error('no package.json file found'))

var mappedConfig = getPublishConfig(
Expand All @@ -109,11 +108,9 @@ function publish_ (arg, data, isRetry, cachedir, cb) {
mapToRegistry(data.name, config, function (er, registryURI, auth, registryBase) {
if (er) return cb(er)

var tarballPath = cachedir + '.tgz'

// we just want the base registry URL in this case
log.verbose('publish', 'registryBase', registryBase)
log.silly('publish', 'uploading', tarballPath)
log.silly('publish', 'uploading', cached)

data._npmUser = {
name: auth.username,
Expand All @@ -122,7 +119,7 @@ function publish_ (arg, data, isRetry, cachedir, cb) {

var params = {
metadata: data,
body: createReadStream(tarballPath),
body: createReadStream(cached),
auth: auth
}

Expand Down
147 changes: 88 additions & 59 deletions test/tap/prepublish-only.js
View file
Original file line number Diff line number Diff line change
@@ -1,76 +1,101 @@
// verify that prepublishOnly runs _only_ on pack and publish
var common = require('../common-tap')
var test = require('tap').test
var fs = require('graceful-fs')
// verify that prepublishOnly runs _only_ on publish
var join = require('path').join
var mkdirp = require('mkdirp')

var mr = require('npm-registry-mock')
var rimraf = require('rimraf')
var test = require('tap').test
var Tacks = require('tacks')
var File = Tacks.File
var Dir = Tacks.Dir

var common = require('../common-tap')

var pkg = join(__dirname, 'prepublish_package')
var tmp = join(pkg, 'tmp')
var cache = join(pkg, 'cache')
var cachedir = join(pkg, 'cache')
var tmpdir = join(pkg, 'tmp')

var env = {
'npm_config_cache': cachedir,
'npm_config_tmp': tmpdir,
'npm_config_prefix': pkg,
'npm_config_global': 'false'
}

for (var i in process.env) {
if (!/^npm_config_/.test(i)) {
env[i] = process.env[i]
}
}

var server

var fixture = new Tacks(Dir({
cache: Dir(),
tmp: Dir(),
'.npmrc': File([
'progress=false',
'registry=' + common.registry,
'//localhost:1337/:username=username',
'//localhost:1337/:_authToken=deadbeeffeed'
].join('\n') + '\n'),
helper: Dir({
'script.js': File([
'#!/usr/bin/env node\n',
'console.log("ok")\n'
].join('\n') + '\n'
),
'package.json': File({
name: 'helper',
version: '6.6.6',
bin: './script.js'
})
}),
'package.json': File({
name: 'npm-test-prepublish-only',
version: '1.2.5',
dependencies: {
'helper': 'file:./helper'
},
scripts: {
build: 'helper',
prepublishOnly: 'npm run build'
}
})
}))

test('setup', function (t) {
var n = 0
cleanup()
mkdirp(pkg, then())
mkdirp(cache, then())
mkdirp(tmp, then())
function then () {
n++
return function (er) {
if (er) throw er
if (--n === 0) next()
}
}
fixture.create(pkg)
mr({port: common.port, throwOnUnmatched: true}, function (err, s) {
t.ifError(err, 'registry mocked successfully')
server = s
common.npm(
[
'install',
'--loglevel', 'error',
'--cache', cachedir,
'--tmp', tmpdir
],
{
cwd: pkg,
env: env
},
function (err, code, stdout, stderr) {
t.equal(code, 0, 'install exited OK')
t.ifErr(err, 'installed successfully')

t.notOk(stderr, 'got stderr data:' + JSON.stringify('' + stderr))

function next () {
fs.writeFile(join(pkg, 'package.json'), JSON.stringify({
name: 'npm-test-prepublish-only',
version: '1.2.5',
scripts: { prepublishOnly: 'echo ok' }
}), 'ascii', function (er) {
if (er) throw er

mr({port: common.port, throwOnUnmatched: true}, function (err, s) {
t.ifError(err, 'registry mocked successfully')
server = s
t.end()
})
})
}
}
)
})
})

test('test', function (t) {
server.filteringRequestBody(function () { return true })
.put('/npm-test-prepublish-only', true)
.reply(201, {ok: true})

var env = {
'npm_config_cache': cache,
'npm_config_tmp': tmp,
'npm_config_prefix': pkg,
'npm_config_global': 'false'
}

for (var i in process.env) {
if (!/^npm_config_/.test(i)) {
env[i] = process.env[i]
}
}

var configuration = [
'progress=false',
'registry=' + common.registry,
'//localhost:1337/:username=username',
'//localhost:1337/:_authToken=deadbeeffeed'
]
var configFile = join(pkg, '.npmrc')

fs.writeFileSync(configFile, configuration.join('\n') + '\n')
common.npm(
[
'publish',
Expand All @@ -81,14 +106,18 @@ test('test', function (t) {
env: env
},
function (err, code, stdout, stderr) {
t.equal(code, 0, 'pack finished successfully')
t.ifErr(err, 'pack finished successfully')
t.equal(code, 0, 'publish ran without error')
t.ifErr(err, 'published successfully')

t.notOk(stderr, 'got stderr data:' + JSON.stringify('' + stderr))
var c = stdout.trim()
var regex = new RegExp(
'> npm-test-prepublish-only@1.2.5 prepublishOnly [^\\r\\n]+\\r?\\n' +
'> echo ok\\r?\\n' +
'> npm run build\\r?\\n' +
'\\r?\\n' +
'\\r?\\n' +
'> npm-test-prepublish-only@1.2.5 build [^\\r\\n]+\\r?\\n' +
'> helper\\r?\\n' +
'\\r?\\n' +
'ok\\r?\\n' +
'\\+ npm-test-prepublish-only@1.2.5', 'ig'
Expand All @@ -108,5 +137,5 @@ test('cleanup', function (t) {
})

function cleanup () {
rimraf.sync(pkg)
fixture.remove(pkg)
}

0 comments on commit 8b32d67

Please sign in to comment.