Migrate to use @inquirer/confirm
#2462
Conversation
lib/utils/index.js
Outdated
|
|
||
| // npm dependencies | ||
| const inquirer = require('inquirer') | ||
| const {default: confirm} = require('@inquirer/confirm') |
There was a problem hiding this comment.
note @inquirer/confirm assigns the actual confirm function to module.exports.default rather than module.exports itself so we need a little renaming here.
| when: () => process.stdout.isTTY, | ||
| default: false | ||
| }]).then(answers => answers.usageData) | ||
| async function askForUsageDataPermission () { |
There was a problem hiding this comment.
note Using async ensures the function returns a Promise if the if block doesn't run. This is not critical as the code calling askForUsageForDataPermission is called with an await rather than using .then, but better safe than sorry.
.then would throw if askForUsageDataPermission returned undefined while await will happily use undefined as a return value on the next tick of the event loop.
Prompts have been removed from `inquirer` since 10.0.0, so we need to migrate our use of `inquirer.prompt` to the standalone `confirm` prompt offered by `@inquirer/prompt`. It does simplify the code a little as the prompt call now returns the users answers, without having to give each prompt a name and picking the value in an `answers` object. The replacement has happened like for like, leaving the check for whether being [run in a "text terminal"](https://nodejs.org/api/tty.html#tty) when asking for the usage data, but not if changing port. It doesn't add any new test either.
romaricpascal
force-pushed
the
use-inquirer-prompts
branch
from
77163db to
75a2af6
Compare
owenatgov
left a comment
owenatgov
left a comment
There was a problem hiding this comment.
Thanks for tidying this up! Sorry for not catching it in my original PR.
This whole saga probably a changelog entry since we're changing a user-facing dependency, but I can handle that next week.
As I understand it, since we fixed it so that There probably should be something in the changelog (especially if this fix goes out by itself. Maybe just something like 'Update from inquirer 8.2.6 to @inquirer/confirm 5.1.15 to address a vulnerability in tmp'? |
We can't use jest.replaceProperty here because in a non-TTY environment the isTTY property _does not exist_ on process.stdout, and Jest will only replace existing properties. The tests ran fine locally but were failing on CI with: “Property `isTTY` does not exist in the provided object”
|
I've pushed a couple of additional commits to add a changelog entry and and also to tests for the 'usage data' side of things – ideally it'd be good to cover |
Note
This is a follow up to changes introduced in #2461
Prompts have been removed from
inquirersince 10.0.0, so we need to migrate our use ofinquirer.promptto the standaloneconfirmprompt offered by@inquirer/prompt.It does simplify the code a little as the prompt call now returns the users answers, without having to give each prompt a name and picking the value in an
answersobject.The replacement has happened like for like, leaving the check for whether being run in a "text terminal" when asking for the usage data, but not if changing port. It doesn't add any new test either.