From 8e45a1ef62dfca3a0f30f6375efc496d57f3ddc1 Mon Sep 17 00:00:00 2001 From: Ghislain B Date: Wed, 27 Jul 2022 15:02:42 -0400 Subject: [PATCH] fix(run-lifecycle): lifecycle events should run to completion in series (#275) * fix(run-lifecycle): lifecycle events should run to completion in series --- packages/core/src/models/interfaces.ts | 12 ++- .../src/utils/__tests__/run-lifecycle.spec.ts | 61 +++++++++----- packages/core/src/utils/run-lifecycle.ts | 82 +++++++++++-------- packages/publish/src/lib/npm-publish.ts | 6 +- packages/publish/src/lib/pack-directory.ts | 5 +- 5 files changed, 102 insertions(+), 64 deletions(-) diff --git a/packages/core/src/models/interfaces.ts b/packages/core/src/models/interfaces.ts index b6480234..e6647b57 100644 --- a/packages/core/src/models/interfaces.ts +++ b/packages/core/src/models/interfaces.ts @@ -53,13 +53,19 @@ export interface ExecOpts { } export interface LifecycleConfig { - log?: typeof log; + access?: 'public' | 'restricted'; + defaultTag?: string; ignorePrepublish?: boolean; ignoreScripts?: boolean; + log: log.Logger; + lernaCommand?: string; nodeOptions?: string; + projectScope?: string | null; scriptShell?: string; scriptsPrependNodePath?: boolean; snapshot?: any; + stdio?: string; + tag?: string; unsafePerm?: boolean; } @@ -82,13 +88,13 @@ export interface UpdateChangelogOption { export interface FetchConfig { fetchRetries: number; - log: typeof log; + log: log.Logger; registry: string; username: string; } export interface PackConfig { - log: typeof log; + log: log.Logger; /* If "publish", run "prepublishOnly" lifecycle */ lernaCommand?: string; diff --git a/packages/core/src/utils/__tests__/run-lifecycle.spec.ts b/packages/core/src/utils/__tests__/run-lifecycle.spec.ts index a66a20b7..c9fb9ab1 100644 --- a/packages/core/src/utils/__tests__/run-lifecycle.spec.ts +++ b/packages/core/src/utils/__tests__/run-lifecycle.spec.ts @@ -1,11 +1,12 @@ jest.mock('@npmcli/run-script', () => jest.fn(() => Promise.resolve({ stdout: '' }))); -const log = require('npmlog'); -const { loggingOutput } = require('@lerna-test/helpers/logging-output'); -const runScript = require('@npmcli/run-script'); -const { npmConf } = require('../npm-conf'); -const { Package } = require('../../package'); -const { runLifecycle, createRunner } = require('../run-lifecycle'); +import log from 'npmlog'; +import { loggingOutput } from '@lerna-test/helpers/logging-output'; +import runScript from '@npmcli/run-script'; +import { npmConf } from '../npm-conf'; +import { Package } from '../../package'; +import { runLifecycle, createRunner } from '../run-lifecycle'; +import { LifecycleConfig } from '../../models'; describe('runLifecycle()', () => { const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); @@ -19,7 +20,7 @@ describe('runLifecycle()', () => { name: 'no-scripts', }; - await runLifecycle(pkg, 'foo', new Map()); + await runLifecycle(pkg as Package, 'foo', new Map() as any); expect(runScript).not.toHaveBeenCalled(); }); @@ -32,7 +33,7 @@ describe('runLifecycle()', () => { }, }; - await runLifecycle(pkg, 'bar', new Map()); + await runLifecycle(pkg as Package, 'bar', new Map() as any); expect(runScript).not.toHaveBeenCalled(); }); @@ -48,13 +49,13 @@ describe('runLifecycle()', () => { engines: { node: '>= 8.9.0', }, - }, + } as unknown as Package, '/test/location' ); const stage = 'preversion'; - const opts = npmConf({ 'custom-cli-flag': true }); + const opts = npmConf({ 'custom-cli-flag': true }) as unknown as LifecycleConfig; - await runLifecycle(pkg, stage, opts); + await runLifecycle(pkg as Package, stage, opts); expect(runScript).toHaveBeenLastCalledWith( expect.objectContaining({ @@ -85,13 +86,13 @@ describe('runLifecycle()', () => { engines: { node: '>= 8.9.0', }, - }, + } as unknown as Package, '/test/location' ); const stage = 'preversion'; const opts = npmConf({ 'custom-cli-flag': true }); - await runLifecycle(pkg, stage, opts); + await runLifecycle(pkg as Package, stage, opts as unknown as LifecycleConfig); expect(consoleSpy).toHaveBeenCalledWith(`\n> test-name@1.0.0-test preversion /test/location\n> test\n`); }); @@ -111,7 +112,7 @@ describe('runLifecycle()', () => { 'script-shell': 'fish', }; - await runLifecycle(pkg, stage, opts); + await runLifecycle(pkg as Package, stage, opts as unknown as LifecycleConfig); expect(runScript).toHaveBeenLastCalledWith( expect.objectContaining({ @@ -135,7 +136,7 @@ describe('runLifecycle()', () => { 'ignore-prepublish': true, }; - await runLifecycle(pkg, stage, opts); + await runLifecycle(pkg as Package, stage, opts as unknown as LifecycleConfig); expect(runScript).not.toHaveBeenCalled(); }); @@ -152,7 +153,7 @@ describe('runLifecycle()', () => { 'ignore-scripts': true, }; - await runLifecycle(pkg, stage, opts); + await runLifecycle(pkg as Package, stage, opts as unknown as LifecycleConfig); expect(runScript).not.toHaveBeenCalled(); }); @@ -169,7 +170,7 @@ describe('runLifecycle()', () => { const stage = 'prepack'; const opts = {}; - await runLifecycle(pkg, stage, opts); + await runLifecycle(pkg as Package, stage, opts as unknown as LifecycleConfig); const callOpts = runScript.mock.calls.pop().pop(); @@ -179,6 +180,7 @@ describe('runLifecycle()', () => { }); describe('createRunner', () => { + const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); const runPackageLifecycle = createRunner({ 'other-cli-flag': 0 }); it('skips missing scripts block', async () => { @@ -188,7 +190,7 @@ describe('createRunner', () => { location: 'test', }; - await runPackageLifecycle(pkg, 'prepare'); + await runPackageLifecycle(pkg as Package, 'prepare'); expect(runScript).not.toHaveBeenCalled(); }); @@ -200,10 +202,27 @@ describe('createRunner', () => { scripts: { test: 'echo foo' }, }; - await runPackageLifecycle(pkg, 'prepare'); + await runPackageLifecycle(pkg as Package, 'prepare'); expect(runScript).not.toHaveBeenCalled(); }); + it('logs stdout from runScript() response', async () => { + runScript.mockImplementationOnce(({ pkg, event }) => { + return Promise.resolve({ stdout: 'runScript output' }); + }); + + const pkg = { + name: 'has-script-error', + version: '1.0.0', + location: 'test', + scripts: { prepublishOnly: 'exit 123' }, + }; + + await runPackageLifecycle(pkg as Package, 'prepublishOnly'); + + expect(consoleSpy).toHaveBeenCalledWith('runScript output'); + }); + it('logs script error and re-throws', async () => { runScript.mockImplementationOnce(({ pkg, event }) => { const err: any = new Error('boom'); @@ -221,7 +240,7 @@ describe('createRunner', () => { scripts: { prepublishOnly: 'exit 123' }, }; - await expect(runPackageLifecycle(pkg, 'prepublishOnly')).rejects.toThrow( + await expect(runPackageLifecycle(pkg as Package, 'prepublishOnly')).rejects.toThrow( expect.objectContaining({ exitCode: 123, script: 'exit 123', @@ -250,7 +269,7 @@ describe('createRunner', () => { scripts: { prepack: 'a-thing-that-ends-poorly' }, }; - await expect(runPackageLifecycle(pkg, 'prepack')).rejects.toThrow( + await expect(runPackageLifecycle(pkg as Package, 'prepack')).rejects.toThrow( expect.objectContaining({ exitCode: 1, script: 'a-thing-that-ends-poorly', diff --git a/packages/core/src/utils/run-lifecycle.ts b/packages/core/src/utils/run-lifecycle.ts index 71794706..7ebd3c3e 100644 --- a/packages/core/src/utils/run-lifecycle.ts +++ b/packages/core/src/utils/run-lifecycle.ts @@ -4,6 +4,9 @@ import runScript from '@npmcli/run-script'; import { npmConf } from '../utils/npm-conf'; import { LifecycleConfig } from '../models'; import { Package } from '../package'; +import PQueue from 'p-queue'; + +const queue = new PQueue({ concurrency: 1 }); /** * Alias dash-cased npmConf to camelCase @@ -46,13 +49,14 @@ export function runLifecycle(pkg: Package, stage: string, options: LifecycleConf } const opts = { + // @ts-ignore log, unsafePerm: true, ...flattenOptions(options), }; const dir = pkg.location; const id = `${pkg.name}@${pkg.version}`; - const config: LifecycleConfig = {}; + const config = {} as LifecycleConfig; if (opts.ignoreScripts) { opts.log.verbose('lifecycle', '%j ignored in %j', stage, pkg.name); @@ -100,45 +104,53 @@ export function runLifecycle(pkg: Package, stage: string, options: LifecycleConf /** * In order to match the previous behavior of 'npm-lifecycle', we have to disable the writing - * to the parent process and print the command banner ourself. + * to the parent process and print the command banner ourselves, unless overridden by the options. */ - const stdio = 'pipe'; + const stdio = opts.stdio || 'pipe'; if (log.level !== 'silent') { printCommandBanner(id, stage, pkg.scripts[stage], dir); } - return runScript({ - event: stage, - path: dir, - pkg, - args: [], - stdio, - banner: false, - scriptShell: config.scriptShell, - }).then( - ({ stdout }) => { - /** - * This adjustment is based on trying to match the existing integration test outputs when migrating - * from 'npm-lifecycle' to '@npmcli/run-script'. - */ - // eslint-disable-next-line no-console - console.log(stdout.toString().trimEnd()); - opts.log.silly('lifecycle', '%j finished in %j', stage, pkg.name); - }, - (err) => { - // propagate the exit code - const exitCode = err.code || 1; - - // error logging has already occurred on stderr, but we need to stop the chain - log.error('lifecycle', '%j errored in %j, exiting %d', stage, pkg.name, exitCode); - // ensure clean logging, avoiding spurious log dump - err.name = 'ValidationError'; - // our yargs.fail() handler expects a numeric .exitCode, not .errno - err.exitCode = exitCode; - process.exitCode = exitCode; - // stop the chain - throw err; - } + return queue.add(async () => + runScript({ + event: stage, + path: dir, + pkg, + args: [], + stdio, + banner: false, + scriptShell: config.scriptShell, + }).then( + ({ stdout }) => { + if (stdout) { + /** + * This adjustment is based on trying to match the existing integration test outputs when migrating + * from "npm-lifecycle" to "@npmcli/run-script". + */ + // eslint-disable-next-line no-console + console.log(stdout.toString().trimEnd()); + } + + opts.log.silly('lifecycle', '%j finished in %j', stage, pkg.name); + }, + (err) => { + // propagate the exit code + const exitCode = err.code || 1; + + // error logging has already occurred on stderr, but we need to stop the chain + log.error('lifecycle', '%j errored in %j, exiting %d', stage, pkg.name, exitCode); + + // ensure clean logging, avoiding spurious log dump + err.name = 'ValidationError'; + + // our yargs.fail() handler expects a numeric .exitCode, not .errno + err.exitCode = exitCode; + process.exitCode = exitCode; + + // stop the chain + throw err; + } + ) ); } diff --git a/packages/publish/src/lib/npm-publish.ts b/packages/publish/src/lib/npm-publish.ts index 9a2f1cc7..92654a5b 100644 --- a/packages/publish/src/lib/npm-publish.ts +++ b/packages/publish/src/lib/npm-publish.ts @@ -6,7 +6,7 @@ import pify from 'pify'; import { publish } from 'libnpmpublish'; import readJSON from 'read-package-json'; -import { OneTimePasswordCache, otplease, Package, RawManifest, runLifecycle } from '@lerna-lite/core'; +import { LifecycleConfig, OneTimePasswordCache, otplease, Package, RawManifest, runLifecycle } from '@lerna-lite/core'; import { LibNpmPublishOptions, PackagePublishConfig } from '../models'; const readJSONAsync = pify(readJSON); @@ -100,8 +100,8 @@ export function npmPublish( }); } - chain = chain.then(() => runLifecycle(pkg, 'publish', opts)); - chain = chain.then(() => runLifecycle(pkg, 'postpublish', opts)); + chain = chain.then(() => runLifecycle(pkg, 'publish', opts as LifecycleConfig)); + chain = chain.then(() => runLifecycle(pkg, 'postpublish', opts as LifecycleConfig)); return chain; } diff --git a/packages/publish/src/lib/pack-directory.ts b/packages/publish/src/lib/pack-directory.ts index 7dd23c0a..e83b9f0b 100644 --- a/packages/publish/src/lib/pack-directory.ts +++ b/packages/publish/src/lib/pack-directory.ts @@ -3,7 +3,7 @@ import packlist from 'npm-packlist'; import log from 'npmlog'; import tar from 'tar'; -import { Package, PackConfig, runLifecycle, tempWrite } from '@lerna-lite/core'; +import { LifecycleConfig, Package, PackConfig, runLifecycle, tempWrite } from '@lerna-lite/core'; import { getPacked } from './get-packed'; import { Readable } from 'stream'; import { Tarball } from '../models'; @@ -16,7 +16,7 @@ import { Tarball } from '../models'; */ export function packDirectory(_pkg: Package, dir: string, options: PackConfig) { const pkg = Package.lazy(_pkg, dir); - const opts = { + const opts: LifecycleConfig = { // @ts-ignore log, ...options, @@ -33,6 +33,7 @@ export function packDirectory(_pkg: Package, dir: string, options: PackConfig) { chain = chain.then(() => runLifecycle(pkg, 'prepare', opts)); if (opts.lernaCommand === 'publish') { + opts.stdio = 'inherit'; chain = chain.then(() => pkg.refresh()); chain = chain.then(() => runLifecycle(pkg, 'prepublishOnly', opts)); chain = chain.then(() => pkg.refresh());