Skip to content
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

fix: revert to original prompt implementation #968

Merged
merged 2 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ jobs:
- https://github.com/salesforcecli/plugin-org
- https://github.com/salesforcecli/plugin-schema
- https://github.com/salesforcecli/plugin-user
- https://github.com/salesforcecli/plugin-settings
with:
packageName: '@oclif/core'
externalProjectGitUrl: ${{ matrix.externalProjectGitUrl }}
Expand Down Expand Up @@ -153,7 +154,7 @@ jobs:
with:
repo: oclif/plugin-plugins
os: ${{ matrix.os }}
command: 'yarn test:integration'
command: yarn test:integration --retries 3
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: external-test could use the normal retries code
salesforcecli/github-workflows/.github/actions/retry instead of all of its consumers need to do this

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll update this

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, actually. I don't think this will work since external-test.yml uses working-directory. An external action (uses) needs to support passing it in, which the retry action we use does not https://github.com/nick-fields/retry

https://stackoverflow.com/questions/58139175/running-actions-in-another-directory#comment126980684_63122434

Copy link
Contributor Author

@mdonnalley mdonnalley Feb 23, 2024

Choose a reason for hiding this comment

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

@iowillhoit @mshanemc I don't think we can use the retry action since you need to be able to set the working_directory, which as far as I can tell isn't possible with that action

Looks like Eric beat me to it 😄

Copy link
Contributor

@iowillhoit iowillhoit Feb 23, 2024

Choose a reason for hiding this comment

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

haha exactly 😅 We were both looking at this at the same time ^

# plugin-plugins integration tests depend on sf being installed globally
other-setup: npm install -g @salesforce/cli@nightly
plugin-update-integration:
Expand All @@ -166,4 +167,4 @@ jobs:
with:
repo: oclif/plugin-update
os: ${{ matrix.os }}
command: 'yarn test:integration:sf'
command: yarn test:integration:sf --retries 3
40 changes: 14 additions & 26 deletions src/cli-ux/prompt.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import chalk from 'chalk'
import readline from 'node:readline'

import * as Errors from '../errors'
import {config} from './config'
Expand Down Expand Up @@ -27,37 +26,26 @@ interface IPromptConfig {

function normal(options: IPromptConfig, retries = 100): Promise<string> {
if (retries < 0) throw new Error('no input')
const ac = new AbortController()
const {signal} = ac

return new Promise((resolve, reject) => {
const rl = readline.createInterface({
input: process.stdin,
output: process.stdout,
})
let timeout: NodeJS.Timeout
// Only set the timeout if the input is a TTY
if (options.timeout && options.isTTY) {
timeout = setTimeout(() => ac.abort(), options.timeout)
signal.addEventListener(
'abort',
() => {
rl.close()
clearTimeout(timeout)
reject(new Error('Prompt timeout'))
},
{once: true},
)
let timer: NodeJS.Timeout
if (options.timeout) {
timer = setTimeout(() => {
process.stdin.pause()
reject(new Error('Prompt timeout'))
}, options.timeout)
timer.unref()
}

rl.question(options.prompt, {signal}, (answer) => {
rl.close()
const data = answer.trim()
process.stdin.setEncoding('utf8')
process.stderr.write(options.prompt)
process.stdin.resume()
process.stdin.once('data', (b) => {
if (timer) clearTimeout(timer)
process.stdin.pause()
const data: string = (typeof b === 'string' ? b : b.toString()).trim()
if (!options.default && options.required && data === '') {
clearTimeout(timeout)
resolve(normal(options, retries - 1))
} else {
clearTimeout(timeout)
resolve(data || (options.default as string))
}
})
Expand Down
127 changes: 62 additions & 65 deletions test/cli-ux/prompt.test.ts
Original file line number Diff line number Diff line change
@@ -1,76 +1,73 @@
import {expect} from 'chai'
import readline from 'node:readline'
import {SinonSandbox, createSandbox} from 'sinon'
import * as chai from 'chai'

const {expect} = chai

import {ux} from '../../src/cli-ux'
import {fancy} from './fancy'

describe('prompt', () => {
let sandbox: SinonSandbox

function stubReadline(answers: string[]) {
let callCount = 0
sandbox.stub(readline, 'createInterface').returns({
// @ts-expect-error because we're stubbing
async question(_message, opts, cb) {
callCount += 1
cb(answers[callCount - 1])
},
close() {},
fancy
.stdout()
.stderr()
.end('requires input', async () => {
const promptPromise = ux.prompt('Require input?')
process.stdin.emit('data', '')
process.stdin.emit('data', 'answer')
const answer = await promptPromise
await ux.done()
expect(answer).to.equal('answer')
})
}

beforeEach(() => {
sandbox = createSandbox()
})

afterEach(() => {
sandbox.restore()
})

it('should require input', async () => {
stubReadline(['', '', 'answer'])
const answer = await ux.prompt('Require input?')
expect(answer).to.equal('answer')
})

it('should not require input if required = false', async () => {
stubReadline([''])
const answer = await ux.prompt('Require input?', {required: false})
expect(answer).to.equal('')
})

it('should use default input', async () => {
stubReadline([''])
const answer = await ux.prompt('Require input?', {default: 'default'})
expect(answer).to.equal('default')
})
fancy
.stdout()
.stderr()
.stdin('y')
.end('confirm', async () => {
const promptPromise = ux.confirm('yes/no?')
const answer = await promptPromise
await ux.done()
expect(answer).to.equal(true)
})

it('should timeout after provided timeout', async () => {
stubReadline([''])
sandbox.stub(process, 'stdin').value({isTTY: true})
try {
await ux.prompt('Require input?', {timeout: 10})
expect.fail('should have thrown')
} catch (error: any) {
expect(error.message).to.equal('Prompt timeout')
}
})
fancy
.stdout()
.stderr()
.stdin('n')
.end('confirm', async () => {
const promptPromise = ux.confirm('yes/no?')
const answer = await promptPromise
await ux.done()
expect(answer).to.equal(false)
})

it('should confirm with y', async () => {
stubReadline(['y'])
const answer = await ux.confirm('yes/no?')
expect(answer).to.equal(true)
})
fancy
.stdout()
.stderr()
.stdin('x')
.end('gets anykey', async () => {
const promptPromise = ux.anykey()
const answer = await promptPromise
await ux.done()
expect(answer).to.equal('x')
})

it('should confirm with n', async () => {
stubReadline(['n'])
const answer = await ux.confirm('yes/no?')
expect(answer).to.equal(false)
})
fancy
.stdout()
.stderr()
.end('does not require input', async () => {
const promptPromise = ux.prompt('Require input?', {
required: false,
})
process.stdin.emit('data', '')
const answer = await promptPromise
await ux.done()
expect(answer).to.equal('')
})

it('should get anykey', async () => {
stubReadline(['x'])
const answer = await ux.anykey()
expect(answer).to.equal('x')
})
fancy
.stdout()
.stderr()
.it('timeouts with no input', async () => {
await expect(ux.prompt('Require input?', {timeout: 1})).to.eventually.be.rejectedWith('Prompt timeout')
})
})
Loading