Skip to content

Commit

Permalink
fix: use promiseSpawn.open instead of opener
Browse files Browse the repository at this point in the history
  • Loading branch information
nlf authored and wraithgar committed Nov 2, 2022
1 parent 41843ad commit 8fd614a
Show file tree
Hide file tree
Showing 11 changed files with 81 additions and 87 deletions.
12 changes: 2 additions & 10 deletions lib/utils/open-url-prompt.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const readline = require('readline')
const opener = require('opener')
const promiseSpawn = require('@npmcli/promise-spawn')

function print (npm, title, url) {
const json = npm.config.get('json')
Expand Down Expand Up @@ -64,15 +64,7 @@ const promptOpen = async (npm, url, title, prompt, emitter) => {
}

const command = browser === true ? null : browser
await new Promise((resolve, reject) => {
opener(url, { command }, err => {
if (err) {
return reject(err)
}

return resolve()
})
})
await promiseSpawn.open(url, { command })
}

module.exports = promptOpen
18 changes: 7 additions & 11 deletions lib/utils/open-url.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const opener = require('opener')
const promiseSpawn = require('@npmcli/promise-spawn')

const { URL } = require('url')

Expand Down Expand Up @@ -37,18 +37,14 @@ const open = async (npm, url, errMsg, isFile) => {
}

const command = browser === true ? null : browser
await new Promise((resolve, reject) => {
opener(url, { command }, (err) => {
if (err) {
if (err.code === 'ENOENT') {
printAlternateMsg()
} else {
return reject(err)
}
await promiseSpawn.open(url, { command })
.catch((err) => {
if (err.code !== 'ENOENT') {
throw err
}
return resolve()

printAlternateMsg()
})
})
}

module.exports = open
1 change: 1 addition & 0 deletions smoke-tests/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ const exec = async (...args) => {
env: {
HOME: path,
PATH: `${PATH}:${binLocation}`,
COMSPEC: process.env.COMSPEC,
},
stdioString: true,
encoding: 'utf-8',
Expand Down
4 changes: 2 additions & 2 deletions test/fixtures/sandbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,14 +166,14 @@ class Sandbox extends EventEmitter {
// replace default config values with placeholders
for (const name of redactedDefaults) {
const value = this[_npm].config.defaults[name]
clean = clean.split(value).join(`{${name.toUpperCase()}}`)
clean = clean.split(normalize(value)).join(`{${name.toUpperCase()}}`)
}

// replace vague default config values that are present within quotes
// with placeholders
for (const name of vagueRedactedDefaults) {
const value = this[_npm].config.defaults[name]
clean = clean.split(`"${value}"`).join(`"{${name.toUpperCase()}}"`)
clean = clean.split(`"${normalize(value)}"`).join(`"{${name.toUpperCase()}}"`)
}
}

Expand Down
43 changes: 23 additions & 20 deletions test/lib/commands/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,11 @@ const { load: loadMockNpm } = require('../../fixtures/mock-npm')

const spawk = tspawk(t)

// 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')

const npmConfig = {
config: {
'ignore-scripts': false,
editor: 'testeditor',
scriptShell: process.platform === 'win32' ? process.env.COMSPEC : 'sh',
},
prefixDir: {
node_modules: {
Expand All @@ -34,30 +30,34 @@ const npmConfig = {
},
}

const isCmdRe = /(?:^|\\)cmd(?:\.exe)?$/i

t.test('npm edit', async t => {
const { npm, joinedOutput } = await loadMockNpm(t, npmConfig)

const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver')
const [scriptShell, scriptArgs] = makeSpawnArgs({
event: 'install',
path: npm.prefix,
cmd: 'testinstall',
})
spawk.spawn('testeditor', [semverPath])

const scriptShell = npm.config.get('scriptShell')
const scriptArgs = isCmdRe.test(scriptShell)
? ['/d', '/s', '/c', 'testinstall']
: ['-c', 'testinstall']
spawk.spawn(scriptShell, scriptArgs, { cwd: semverPath })

await npm.exec('edit', ['semver'])
t.match(joinedOutput(), 'rebuilt dependencies successfully')
})

t.test('rebuild failure', async t => {
const { npm } = await loadMockNpm(t, npmConfig)

const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver')
const [scriptShell, scriptArgs] = makeSpawnArgs({
event: 'install',
path: npm.prefix,
cmd: 'testinstall',
})
spawk.spawn('testeditor', [semverPath])

const scriptShell = npm.config.get('scriptShell')
const scriptArgs = isCmdRe.test(scriptShell)
? ['/d', '/s', '/c', 'testinstall']
: ['-c', 'testinstall']
spawk.spawn(scriptShell, scriptArgs, { cwd: semverPath }).exit(1).stdout('test error')
await t.rejects(
npm.exec('edit', ['semver']),
Expand All @@ -67,8 +67,10 @@ t.test('rebuild failure', async t => {

t.test('editor failure', async t => {
const { npm } = await loadMockNpm(t, npmConfig)

const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver')
spawk.spawn('testeditor', [semverPath]).exit(1).stdout('test editor failure')

await t.rejects(
npm.exec('edit', ['semver']),
{ message: 'editor process exited with code: 1' }
Expand All @@ -85,13 +87,14 @@ t.test('npm edit editor has flags', async t => {
})

const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver')
const [scriptShell, scriptArgs] = makeSpawnArgs({
event: 'install',
path: npm.prefix,
cmd: 'testinstall',
})
spawk.spawn('testeditor', ['--flag', semverPath])

const scriptShell = npm.config.get('scriptShell')
const scriptArgs = isCmdRe.test(scriptShell)
? ['/d', '/s', '/c', 'testinstall']
: ['-c', 'testinstall']
spawk.spawn(scriptShell, scriptArgs, { cwd: semverPath })

await npm.exec('edit', ['semver'])
})

Expand Down
18 changes: 7 additions & 11 deletions test/lib/commands/restart.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ const { load: loadMockNpm } = require('../../fixtures/mock-npm')

const spawk = tspawk(t)

// 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')
const isCmdRe = /(?:^|\\)cmd(?:\.exe)?$/i

t.test('should run restart script from package.json', async t => {
const { npm } = await loadMockNpm(t, {
Expand All @@ -22,15 +19,14 @@ t.test('should run restart script from package.json', async t => {
},
config: {
loglevel: 'silent',
scriptShell: process.platform === 'win32' ? process.env.COMSPEC : 'sh',
},
})
const [scriptShell, scriptArgs] = makeSpawnArgs({
path: npm.prefix,
cmd: 'node ./test-restart.js',
})
let scriptContent = scriptArgs.pop()
scriptContent += ' foo'
scriptArgs.push(scriptContent)

const scriptShell = npm.config.get('scriptShell')
const scriptArgs = isCmdRe.test(scriptShell)
? ['/d', '/s', '/c', 'node ./test-restart.js foo']
: ['-c', 'node ./test-restart.js foo']
const script = spawk.spawn(scriptShell, scriptArgs)
await npm.exec('restart', ['foo'])
t.ok(script.called, 'script ran')
Expand Down
16 changes: 7 additions & 9 deletions test/lib/commands/start.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ const { load: loadMockNpm } = require('../../fixtures/mock-npm')

const spawk = tspawk(t)

// 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')
const isCmdRe = /(?:^|\\)cmd(?:\.exe)?$/i

t.test('should run start script from package.json', async t => {
const { npm } = await loadMockNpm(t, {
Expand All @@ -22,13 +19,14 @@ t.test('should run start script from package.json', async t => {
},
config: {
loglevel: 'silent',
scriptShell: process.platform === 'win32' ? process.env.COMSPEC : 'sh',
},
})
const [scriptShell, scriptArgs] = makeSpawnArgs({ path: npm.prefix, cmd: 'node ./test-start.js' })
// we're calling the script with 'foo' as an argument, so add that to the script
let scriptContent = scriptArgs.pop()
scriptContent += ' foo'
scriptArgs.push(scriptContent)

const scriptShell = npm.config.get('scriptShell')
const scriptArgs = isCmdRe.test(scriptShell)
? ['/d', '/s', '/c', 'node ./test-start.js foo']
: ['-c', 'node ./test-start.js foo']
const script = spawk.spawn(scriptShell, scriptArgs)
await npm.exec('start', ['foo'])
t.ok(script.called, 'script ran')
Expand Down
15 changes: 7 additions & 8 deletions test/lib/commands/stop.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ const { load: loadMockNpm } = require('../../fixtures/mock-npm')

const spawk = tspawk(t)

// 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')
const isCmdRe = /(?:^|\\)cmd(?:\.exe)?$/i

t.test('should run stop script from package.json', async t => {
const { npm } = await loadMockNpm(t, {
Expand All @@ -22,12 +19,14 @@ t.test('should run stop script from package.json', async t => {
},
config: {
loglevel: 'silent',
scriptShell: process.platform === 'win32' ? process.env.COMSPEC : 'sh',
},
})
const [scriptShell, scriptArgs] = makeSpawnArgs({ path: npm.prefix, cmd: 'node ./test-stop.js' })
let scriptContent = scriptArgs.pop()
scriptContent += ' foo'
scriptArgs.push(scriptContent)

const scriptShell = npm.config.get('scriptShell')
const scriptArgs = isCmdRe.test(scriptShell)
? ['/d', '/s', '/c', 'node ./test-stop.js foo']
: ['-c', 'node ./test-stop.js foo']
const script = spawk.spawn(scriptShell, scriptArgs)
await npm.exec('stop', ['foo'])
t.ok(script.called, 'script ran')
Expand Down
15 changes: 7 additions & 8 deletions test/lib/commands/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ const { load: loadMockNpm } = require('../../fixtures/mock-npm')

const spawk = tspawk(t)

// 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')
const isCmdRe = /(?:^|\\)cmd(?:\.exe)?$/i

t.test('should run test script from package.json', async t => {
const { npm } = await loadMockNpm(t, {
Expand All @@ -22,12 +19,14 @@ t.test('should run test script from package.json', async t => {
},
config: {
loglevel: 'silent',
scriptShell: process.platform === 'win32' ? process.env.COMSPEC : 'sh',
},
})
const [scriptShell, scriptArgs] = makeSpawnArgs({ path: npm.prefix, cmd: 'node ./test-test.js' })
let scriptContent = scriptArgs.pop()
scriptContent += ' foo'
scriptArgs.push(scriptContent)

const scriptShell = npm.config.get('scriptShell')
const scriptArgs = isCmdRe.test(scriptShell)
? ['/d', '/s', '/c', 'node ./test-test.js foo']
: ['-c', 'node ./test-test.js foo']
const script = spawk.spawn(scriptShell, scriptArgs)
await npm.exec('test', ['foo'])
t.ok(script.called, 'script ran')
Expand Down
13 changes: 9 additions & 4 deletions test/lib/utils/open-url-prompt.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@ const npm = {
let openerUrl = null
let openerOpts = null
let openerResult = null
const opener = (url, opts, cb) => {

const open = async (url, options) => {
openerUrl = url
openerOpts = opts
return cb(openerResult)
openerOpts = options
if (openerResult) {
throw openerResult
}
}

let questionShouldResolve = true
Expand All @@ -47,7 +50,9 @@ const readline = {
}

const openUrlPrompt = t.mock('../../../lib/utils/open-url-prompt.js', {
opener,
'@npmcli/promise-spawn': {
open,
},
readline,
})

Expand Down
13 changes: 9 additions & 4 deletions test/lib/utils/open-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,19 @@ const npm = {
let openerUrl = null
let openerOpts = null
let openerResult = null
const opener = (url, opts, cb) => {

const open = async (url, options) => {
openerUrl = url
openerOpts = opts
return cb(openerResult)
openerOpts = options
if (openerResult) {
throw openerResult
}
}

const openUrl = t.mock('../../../lib/utils/open-url.js', {
opener,
'@npmcli/promise-spawn': {
open,
},
})

t.test('opens a url', async t => {
Expand Down

0 comments on commit 8fd614a

Please sign in to comment.