Skip to content

Commit

Permalink
fix: don't set stdioString for any spawn/run-script calls
Browse files Browse the repository at this point in the history 
These libraries now return strings by default which is what we always
want in the CLI for error reporting.

Fixes npm#5766
  • Loading branch information
@lukekarrys @wraithgar
lukekarrys authored and wraithgar committed Nov 2, 2022
1 parent abfb28b commit 1f5382d
Show file tree
Hide file tree
Showing 14 changed files with 9 additions and 40 deletions.
1 change: 0 additions & 1 deletion lib/commands/ci.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ class CI extends ArboristWorkspaceCmd {
args: [],
scriptShell,
stdio: 'inherit',
stdioString: true,
banner: !this.npm.silent,
event,
})
Expand Down
1 change: 0 additions & 1 deletion lib/commands/explore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ class Explore extends BaseCommand {
pkg,
banner: false,
path,
stdioString: true,
event: '_explore',
stdio: 'inherit',
}).catch(er => {
Expand Down
1 change: 0 additions & 1 deletion lib/commands/install.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ class Install extends ArboristWorkspaceCmd {
args: [],
scriptShell,
stdio: 'inherit',
stdioString: true,
banner: !this.npm.silent,
event,
})
Expand Down
1 change: 0 additions & 1 deletion lib/commands/run-script.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ class RunScript extends BaseCommand {
args,
scriptShell,
stdio: 'inherit',
stdioString: true,
pkg,
banner: !this.npm.silent,
}
Expand Down
1 change: 0 additions & 1 deletion scripts/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ const spawn = async (cmd, ...allArgs) => {
let res = null
try {
const spawnOpts = {
stdioString: true,
stdio: quiet || out || lines ? 'pipe' : 'inherit',
cwd: CWD,
...opts,
Expand Down
1 change: 0 additions & 1 deletion smoke-tests/test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ const exec = async (...args) => {
PATH: `${PATH}:${binLocation}`,
COMSPEC: process.env.COMSPEC,
},
stdioString: true,
encoding: 'utf-8',
})

Expand Down
2 changes: 0 additions & 2 deletions test/bin/windows-shims.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ for (const [name, bash] of bashes) {
const result = await spawn(bash, args, {
env: { PATH: path, npm_config_update_notifier: 'false' },
cwd: path,
stdioString: true,
})
t.match(result, {
cmd: bash,
Expand All @@ -119,7 +118,6 @@ for (const [name, bash] of bashes) {
const result = await spawn(bash, args, {
env: { PATH: path, npm_config_update_notifier: 'false' },
cwd: path,
stdioString: true,
})
t.match(result, {
cmd: bash,
Expand Down
9 changes: 0 additions & 9 deletions test/lib/commands/run-script.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ t.test('default env, start, and restart scripts', t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: { name: 'x', version: '1.2.3', _id: '[email protected]', scripts: {} },
event: 'start',
},
Expand All @@ -140,7 +139,6 @@ t.test('default env, start, and restart scripts', t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand All @@ -162,7 +160,6 @@ t.test('default env, start, and restart scripts', t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand All @@ -185,7 +182,6 @@ t.test('default env, start, and restart scripts', t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand Down Expand Up @@ -220,7 +216,6 @@ t.test('non-default env script', t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand All @@ -242,7 +237,6 @@ t.test('non-default env script', t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand Down Expand Up @@ -305,7 +299,6 @@ t.test('run pre/post hooks', async t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand Down Expand Up @@ -342,7 +335,6 @@ t.test('skip pre/post hooks when using ignoreScripts', async t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand Down Expand Up @@ -385,7 +377,6 @@ t.test('run silent', async t => {
args: [],
scriptShell: undefined,
stdio: 'inherit',
stdioString: true,
pkg: {
name: 'x',
version: '1.2.3',
Expand Down
1 change: 0 additions & 1 deletion workspaces/arborist/lib/arborist/rebuild.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,6 @@ module.exports = cls => class Builder extends cls {
event,
path,
pkg,
stdioString: true,
stdio,
env,
scriptShell: this[_scriptShell],
Expand Down
1 change: 0 additions & 1 deletion workspaces/arborist/lib/arborist/reify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1558,7 +1558,6 @@ module.exports = cls => class Reifier extends cls {
event,
path,
pkg,
stdioString: true,
stdio,
scriptShell: this.options.scriptShell,
})
Expand Down
1 change: 0 additions & 1 deletion workspaces/arborist/test/arborist/rebuild.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,6 @@ t.test('rebuild node-gyp dependencies lacking both preinstall and install script
event: 'install',
path: resolve(path, 'node_modules/dep'),
pkg: { scripts: { install: 'node-gyp rebuild' } },
stdioString: true,
env: {
npm_package_resolved: null,
npm_package_integrity: null,
Expand Down
1 change: 0 additions & 1 deletion workspaces/arborist/test/arborist/reify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1770,7 +1770,6 @@ console.log('ok 1 - this is fine')
event: 'test',
path,
pkg,
stdioString: true,
stdio: 'pipe',
}), 'test result')
})
Expand Down
1 change: 0 additions & 1 deletion workspaces/libnpmexec/lib/run-script.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ const run = async ({
banner: false,
// we always run in cwd, not --prefix
path: runPath,
stdioString: true,
binPaths,
event: 'npx',
args,
Expand Down
27 changes: 9 additions & 18 deletions workspaces/libnpmpack/test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ const OPTS = {

const REG = OPTS.registry

// TODO this ... smells. npm "script-shell" config mentions defaults but those
// are handled by run-script, not npm. So for now we have to tie tests to some
// pretty specific internals of runScript
const makeSpawnArgs = require('@npmcli/run-script/lib/make-spawn-args.js')

t.test('packs from local directory', async t => {
const testDir = t.testdir({
'package.json': JSON.stringify({
Expand Down Expand Up @@ -152,13 +147,15 @@ t.test('runs scripts in foreground when foregroundScripts === true', async t =>
const cwd = process.cwd()
process.chdir(testDir)

const [scriptShell, scriptArgs] = makeSpawnArgs({
event: 'prepack',
path: testDir,
cmd: 'touch prepack',
})
const shell = process.platform === 'win32'
? process.env.COMSPEC
: 'sh'

const args = process.platform === 'win32'
? ['/d', '/s', '/c', 'touch prepack']
: ['-c', 'touch prepack']

const prepack = spawk.spawn(scriptShell, scriptArgs)
const prepack = spawk.spawn(shell, args)

await pack('file:.', {
packDestination: testDir,
Expand Down Expand Up @@ -186,13 +183,7 @@ t.test('doesn\'t run scripts when ignoreScripts === true', async t => {
const cwd = process.cwd()
process.chdir(testDir)

const [scriptShell, scriptArgs] = makeSpawnArgs({
event: 'prepack',
path: testDir,
cmd: 'touch prepack',
})

const prepack = spawk.spawn(scriptShell, scriptArgs)
const prepack = spawk.spawn('sh', ['-c', 'touch prepack'])

await pack('file:.', {
packDestination: testDir,
Expand Down

0 comments on commit 1f5382d

Please sign in to comment.