From 0cf39d171c3d6ce2e25a00884e308d61b5348c45 Mon Sep 17 00:00:00 2001 From: Jordan Harband Date: Thu, 18 Jan 2024 15:17:57 -0800 Subject: [PATCH] fix(pack, publish): default foreground-scripts to true Fixes #6816 --- lib/commands/pack.js | 3 + lib/commands/publish.js | 3 + .../test/lib/commands/pack.js.test.cjs | 42 +++++++++ .../test/lib/commands/publish.js.test.cjs | 86 +++++++++++++++++++ tap-snapshots/test/lib/docs.js.test.cjs | 3 +- test/lib/commands/pack.js | 81 +++++++++++++++++ test/lib/commands/publish.js | 86 +++++++++++++++++++ .../config/lib/definitions/definitions.js | 2 + 8 files changed, 305 insertions(+), 1 deletion(-) diff --git a/lib/commands/pack.js b/lib/commands/pack.js index 74e80e573c2e9..6d5f00df55e3f 100644 --- a/lib/commands/pack.js +++ b/lib/commands/pack.js @@ -47,6 +47,9 @@ class Pack extends BaseCommand { for (const { arg, manifest } of manifests) { const tarballData = await libpack(arg, { ...this.npm.flatOptions, + foregroundScripts: this.npm.config.isDefault('foreground-scripts') + ? true + : this.npm.config.get('foreground-scripts'), prefix: this.npm.localPrefix, workspaces: this.workspacePaths, }) diff --git a/lib/commands/publish.js b/lib/commands/publish.js index 7b3e930922eca..63abc50b4745f 100644 --- a/lib/commands/publish.js +++ b/lib/commands/publish.js @@ -80,6 +80,9 @@ class Publish extends BaseCommand { // we pass dryRun: true to libnpmpack so it doesn't write the file to disk const tarballData = await pack(spec, { ...opts, + foregroundScripts: this.npm.config.isDefault('foreground-scripts') + ? true + : this.npm.config.get('foreground-scripts'), dryRun: true, prefix: this.npm.localPrefix, workspaces: this.workspacePaths, diff --git a/tap-snapshots/test/lib/commands/pack.js.test.cjs b/tap-snapshots/test/lib/commands/pack.js.test.cjs index 2e4fdf499a93e..d2148c40bd5df 100644 --- a/tap-snapshots/test/lib/commands/pack.js.test.cjs +++ b/tap-snapshots/test/lib/commands/pack.js.test.cjs @@ -26,6 +26,48 @@ Array [ ] ` +exports[`test/lib/commands/pack.js TAP foreground-scripts can still be set to false > logs pack contents 1`] = ` +Array [ + undefined, + "package: test-fg-scripts@0.0.0", + undefined, + "110B package.json", + undefined, + String( + name: test-fg-scripts + version: 0.0.0 + filename: test-fg-scripts-0.0.0.tgz + package size: {size} + unpacked size: 110 B + shasum: {sha} + integrity: {integrity} + total files: 1 + ), + "", +] +` + +exports[`test/lib/commands/pack.js TAP foreground-scripts defaults to true > logs pack contents 1`] = ` +Array [ + undefined, + "package: test-fg-scripts@0.0.0", + undefined, + "110B package.json", + undefined, + String( + name: test-fg-scripts + version: 0.0.0 + filename: test-fg-scripts-0.0.0.tgz + package size: {size} + unpacked size: 110 B + shasum: {sha} + integrity: {integrity} + total files: 1 + ), + "", +] +` + exports[`test/lib/commands/pack.js TAP should log output as valid json > logs pack contents 1`] = ` Array [] ` diff --git a/tap-snapshots/test/lib/commands/publish.js.test.cjs b/tap-snapshots/test/lib/commands/publish.js.test.cjs index 5d7582031b67a..1bbdebb4fd617 100644 --- a/tap-snapshots/test/lib/commands/publish.js.test.cjs +++ b/tap-snapshots/test/lib/commands/publish.js.test.cjs @@ -56,6 +56,92 @@ Array [ ] ` +exports[`test/lib/commands/publish.js TAP foreground-scripts can still be set to false > must match snapshot 1`] = ` +Array [ + Array [ + "", + ], + Array [ + "", + "package: test-fg-scripts@0.0.0", + ], + Array [ + "=== Tarball Contents ===", + ], + Array [ + "", + "110B package.json", + ], + Array [ + "=== Tarball Details ===", + ], + Array [ + "", + String( + name: test-fg-scripts + version: 0.0.0 + filename: test-fg-scripts-0.0.0.tgz + package size: {size} + unpacked size: 110 B + shasum: {sha} + integrity: {integrity} + total files: 1 + ), + ], + Array [ + "", + "", + ], + Array [ + "", + "Publishing to https://registry.npmjs.org/ with tag latest and default access (dry-run)", + ], +] +` + +exports[`test/lib/commands/publish.js TAP foreground-scripts defaults to true > must match snapshot 1`] = ` +Array [ + Array [ + "", + ], + Array [ + "", + "package: test-fg-scripts@0.0.0", + ], + Array [ + "=== Tarball Contents ===", + ], + Array [ + "", + "110B package.json", + ], + Array [ + "=== Tarball Details ===", + ], + Array [ + "", + String( + name: test-fg-scripts + version: 0.0.0 + filename: test-fg-scripts-0.0.0.tgz + package size: {size} + unpacked size: 110 B + shasum: {sha} + integrity: {integrity} + total files: 1 + ), + ], + Array [ + "", + "", + ], + Array [ + "", + "Publishing to https://registry.npmjs.org/ with tag latest and default access (dry-run)", + ], +] +` + exports[`test/lib/commands/publish.js TAP has mTLS auth for scope configured registry > new package version 1`] = ` + @npm/test-package@1.0.0 ` diff --git a/tap-snapshots/test/lib/docs.js.test.cjs b/tap-snapshots/test/lib/docs.js.test.cjs index cf5b74eeda843..7490d818ab43b 100644 --- a/tap-snapshots/test/lib/docs.js.test.cjs +++ b/tap-snapshots/test/lib/docs.js.test.cjs @@ -637,7 +637,8 @@ recommended that you do not use this option! #### \`foreground-scripts\` -* Default: false +* Default: \`false\` unless when using \`npm pack\` or \`npm publish\` where it + defaults to \`true\` * Type: Boolean Run all build scripts (ie, \`preinstall\`, \`install\`, and \`postinstall\`) diff --git a/test/lib/commands/pack.js b/test/lib/commands/pack.js index 658fd3489abea..baec163c7b34d 100644 --- a/test/lib/commands/pack.js +++ b/test/lib/commands/pack.js @@ -105,6 +105,87 @@ t.test('dry run', async t => { t.throws(() => fs.statSync(path.resolve(npm.prefix, filename))) }) +t.test('foreground-scripts defaults to true', async t => { + const { npm, outputs, logs } = await loadMockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify({ + name: 'test-fg-scripts', + version: '0.0.0', + scripts: { + prepack: 'echo prepack!', + postpack: 'echo postpack!', + }, + } + ), + }, + config: { 'dry-run': true }, + }) + + /* eslint no-console: 0 */ + // TODO: replace this with `const results = t.intercept(console, 'log')` + const log = console.log + t.teardown(() => { + console.log = log + }) + const caughtLogs = [] + console.log = (...args) => { + caughtLogs.push(args) + } + // end TODO + + await npm.exec('pack', []) + const filename = 'test-fg-scripts-0.0.0.tgz' + t.same( + caughtLogs, + [ + ['\n> test-fg-scripts@0.0.0 prepack\n> echo prepack!\n'], + ['\n> test-fg-scripts@0.0.0 postpack\n> echo postpack!\n'], + ], + 'prepack and postpack log to stdout') + t.strictSame(outputs, [[filename]]) + t.matchSnapshot(logs.notice.map(([, m]) => m), 'logs pack contents') + t.throws(() => fs.statSync(path.resolve(npm.prefix, filename))) +}) + +t.test('foreground-scripts can still be set to false', async t => { + const { npm, outputs, logs } = await loadMockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify({ + name: 'test-fg-scripts', + version: '0.0.0', + scripts: { + prepack: 'echo prepack!', + postpack: 'echo postpack!', + }, + } + ), + }, + config: { 'dry-run': true, 'foreground-scripts': false }, + }) + + /* eslint no-console: 0 */ + // TODO: replace this with `const results = t.intercept(console, 'log')` + const log = console.log + t.teardown(() => { + console.log = log + }) + const caughtLogs = [] + console.log = (...args) => { + caughtLogs.push(args) + } + // end TODO + + await npm.exec('pack', []) + const filename = 'test-fg-scripts-0.0.0.tgz' + t.same( + caughtLogs, + [], + 'prepack and postpack do not log to stdout') + t.strictSame(outputs, [[filename]]) + t.matchSnapshot(logs.notice.map(([, m]) => m), 'logs pack contents') + t.throws(() => fs.statSync(path.resolve(npm.prefix, filename))) +}) + t.test('invalid packument', async t => { const { npm, outputs } = await loadMockNpm(t, { prefixDir: { diff --git a/test/lib/commands/publish.js b/test/lib/commands/publish.js index a5644ce224d67..ec7299e9eec53 100644 --- a/test/lib/commands/publish.js +++ b/test/lib/commands/publish.js @@ -167,6 +167,92 @@ t.test('dry-run', async t => { t.matchSnapshot(logs.notice) }) +t.test('foreground-scripts defaults to true', async t => { + const { joinedOutput, npm, logs } = await loadMockNpm(t, { + config: { + 'dry-run': true, + ...auth, + }, + prefixDir: { + 'package.json': JSON.stringify({ + name: 'test-fg-scripts', + version: '0.0.0', + scripts: { + prepack: 'echo prepack!', + postpack: 'echo postpack!', + }, + } + ), + }, + }) + + /* eslint no-console: 0 */ + // TODO: replace this with `const results = t.intercept(console, 'log')` + const log = console.log + t.teardown(() => { + console.log = log + }) + const caughtLogs = [] + console.log = (...args) => { + caughtLogs.push(args) + } + // end TODO + + await npm.exec('publish', []) + t.equal(joinedOutput(), `+ test-fg-scripts@0.0.0`) + t.matchSnapshot(logs.notice) + + t.same( + caughtLogs, + [ + ['\n> test-fg-scripts@0.0.0 prepack\n> echo prepack!\n'], + ['\n> test-fg-scripts@0.0.0 postpack\n> echo postpack!\n'], + ], + 'prepack and postpack log to stdout') +}) + +t.test('foreground-scripts can still be set to false', async t => { + const { joinedOutput, npm, logs } = await loadMockNpm(t, { + config: { + 'dry-run': true, + 'foreground-scripts': false, + ...auth, + }, + prefixDir: { + 'package.json': JSON.stringify({ + name: 'test-fg-scripts', + version: '0.0.0', + scripts: { + prepack: 'echo prepack!', + postpack: 'echo postpack!', + }, + } + ), + }, + }) + + /* eslint no-console: 0 */ + // TODO: replace this with `const results = t.intercept(console, 'log')` + const log = console.log + t.teardown(() => { + console.log = log + }) + const caughtLogs = [] + console.log = (...args) => { + caughtLogs.push(args) + } + // end TODO + + await npm.exec('publish', []) + t.equal(joinedOutput(), `+ test-fg-scripts@0.0.0`) + t.matchSnapshot(logs.notice) + + t.same( + caughtLogs, + [], + 'prepack and postpack do not log to stdout') +}) + t.test('shows usage with wrong set of arguments', async t => { const { publish } = await loadMockNpm(t, { command: 'publish' }) await t.rejects(publish.exec(['a', 'b', 'c']), publish.usage) diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index 1f320dfc9d9b6..09b0eceeea6b2 100644 --- a/workspaces/config/lib/definitions/definitions.js +++ b/workspaces/config/lib/definitions/definitions.js @@ -786,6 +786,8 @@ define('force', { define('foreground-scripts', { default: false, + defaultDescription: `\`false\` unless when using \`npm pack\` or \`npm publish\` where it + defaults to \`true\``, type: Boolean, description: ` Run all build scripts (ie, \`preinstall\`, \`install\`, and