Skip to content

fix: the spinner #8322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: latest
Choose a base branch
from
35 changes: 29 additions & 6 deletions lib/utils/display.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ class Display {

// progress
#progress
#silentPrompt

// options
#command
Expand Down Expand Up @@ -347,6 +348,11 @@ class Display {

case input.KEYS.end:
log.resume()
if (this.#silentPrompt) {
// Add newline to preserve output
output.standard('')
this.#silentPrompt = false
}
output.flush()
this.#progress.resume()
break
Expand All @@ -355,12 +361,29 @@ class Display {
// The convention when calling input.read is to pass in a single fn that returns
// the promise to await. resolve and reject are provided by proc-log
const [res, rej, p] = args
return input.start(() => p()
.then(res)
.catch(rej)
// Any call to procLog.input.read will render a prompt to the user, so we always
// add a single newline of output to stdout to move the cursor to the next line
.finally(() => output.standard('')))

// Silent inputs like password prompts require special handling
// to preserve output when users hit enter (see input.KEYS.end).
this.#silentPrompt = meta[META]?.silentPrompt || false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me nervous. The display class is global, and state should be for the entire npm process, not for a single event. It will likely mean we need to pass this along as we are calling the input events.

It seems here we have a more subtle issue in that proc-log itself isn't aware of META in the input start/end lifecycle. It's an oversight and one we likely need to fix there instead. Then our input handler in the display layer can rely on having the META passed into it as originally requested.

TLDR: the real problem here is that we need to ensure that META is maintained and passed along through the entire input lifecycle so that input.KEYS.end has access to it as originally requested.


// Use sequential input management to avoid race condition which causes issues
// with spinner and adding newlines
process.emit('input', 'start')

return p()
.then((result) => {
// User hits enter, process end event and return input
process.emit('input', 'end')
res(result)
return result
})
.catch((error) => {
// User hits ctrl+c, add newline to preserve output
output.standard('')
this.#silentPrompt = false
process.emit('input', 'end')
rej(error)
})
}
}
})
Expand Down
8 changes: 6 additions & 2 deletions lib/utils/read-user-info.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const { read: _read } = require('read')
const userValidate = require('npm-user-validate')
const { log, input } = require('proc-log')
const { log, input, META } = require('proc-log')

const otpPrompt = `This command requires a one-time password (OTP) from your authenticator app.
Enter one below. You can also pass one on the command line by appending --otp=123456.
Expand All @@ -11,7 +11,11 @@ const passwordPrompt = 'npm password: '
const usernamePrompt = 'npm username: '
const emailPrompt = 'email (this IS public): '

const read = (...args) => input.read(() => _read(...args))
const read = (...args) => {
// Pass silent information through to determine if we need to add a newline after the prompt
// Rename parameter to avoid confusion with silent logging in Display
return input.read(() => _read(...args), { [META]: { silentPrompt: args[0]?.silent } })
}

function readOTP (msg = otpPrompt, otp, isRetry) {
if (isRetry && otp && /^[\d ]+$|^[A-Fa-f0-9]{64,64}$/.test(otp)) {
Expand Down
1 change: 0 additions & 1 deletion tap-snapshots/test/lib/commands/init.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,5 @@ Press ^C at any time to quit.

exports[`test/lib/commands/init.js TAP workspaces no args -- yes > should print helper info 1`] = `


added 1 package in {TIME}
`
1 change: 0 additions & 1 deletion tap-snapshots/test/lib/utils/open-url.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ https://www.npmjs.com
exports[`test/lib/utils/open-url.js TAP open url prompt does not error when opener can not find command > Outputs extra Browser unavailable message and url 1`] = `
npm home:
https://www.npmjs.com

Browser unavailable. Please open the URL manually:
https://www.npmjs.com
`
Expand Down
43 changes: 43 additions & 0 deletions test/lib/utils/display.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,46 @@ t.test('Display.clean', async (t) => {
clearOutput()
}
})

t.test('prompt functionality', async t => {
t.test('regular prompt completion works', async t => {
const { input } = await mockDisplay(t)

const result = await input.read(() => Promise.resolve('user-input'))

t.equal(result, 'user-input', 'should return the input result')
})

t.test('silent prompt completion works', async t => {
const { input, META } = await mockDisplay(t)

const result = await input.read(
() => Promise.resolve('secret-password'),
{ [META]: { silentPrompt: true } }
)

t.equal(result, 'secret-password', 'should return the input result for silent prompts')
})

t.test('metadata is correctly passed through', async t => {
const { input, META } = await mockDisplay(t)

await input.read(
() => Promise.resolve('result1'),
{ [META]: { silentPrompt: false } }
)
t.pass('should handle silentPrompt: false metadata')

await input.read(
() => Promise.resolve('result2'),
{ [META]: {} }
)
t.pass('should handle empty metadata')

await input.read(
() => Promise.resolve('result3'),
{ [META]: { silentPrompt: true } }
)
t.pass('should handle silentPrompt: true metadata')
})
})
32 changes: 32 additions & 0 deletions test/lib/utils/read-user-info.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,35 @@ t.test('email - invalid warns and retries', async (t) => {
t.equal(result, '[email protected]', 'received the email')
t.equal(logMsg, 'invalid email')
})

t.test('read-user-info integration works', async (t) => {
t.teardown(() => {
readResult = null
readOpts = null
})

readResult = 'regular-input'
const username = await readUserInfo.username('Username: ')
t.equal(username, 'regular-input', 'should return username from regular prompt')
t.notOk(readOpts.silent, 'username prompt should not set silent')

readResult = 'secret-password'
const password = await readUserInfo.password('Password: ')
t.equal(password, 'secret-password', 'should return password from silent prompt')
t.match(readOpts, { silent: true }, 'password prompt should set silent: true')
})

t.test('silent metadata is passed correctly by read-user-info', async (t) => {
t.teardown(() => {
readResult = null
readOpts = null
})

readResult = 'username'
await readUserInfo.username('Username: ')
t.notOk(readOpts?.silent, 'username prompt should not set silent')

readResult = 'password'
await readUserInfo.password('Password: ')
t.equal(readOpts?.silent, true, 'password prompt should set silent: true')
})
Loading