-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
perf: switch to clack for CLI prompts #14589
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
base: main
Are you sure you want to change the base?
Conversation
This is an incremental change as part of the wider move to clack.
Switches to using clack for rendering boxes in the terminal. Part of the wider migration to clack.
Switches to using clack for confirm prompts rather than `prompts` package. Part of the wider clack migration.
|
CodSpeed Performance ReportMerging #14589 will not alter performanceComparing Summary
Footnotes |
Noting before I forget that a good follow-up/companion to this would be replacing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @43081j! The code changes all look good and the logic seems correct! I went through some of the prompts comparing before/after to get an idea of what will change and left a few thoughts.
Clack prompts introduce a left border/margin. We need to decide if this is ok or not. If it is not ok, that is fine as we can still figure it out (whether it be here or in clack itself)
Good question. I quite like the border/margin style, but having run through a few commands, it does leave things feeling a little inconsistent: logging not done via Clack often has a different margin and no border, so it would be nice to make that consistent one way or another.
Assuming it’s an easily centralised option to control Clack’s styles, maybe the easiest is just to avoid the borders given there are some situations (astro info
for example) where a border needs to be avoided?
The boxes were previously logged via the logger, but are now immediately logged to stdout. This means we will lose the abstraction the logger provides over
console
vsstdout
. If we want the logger to work with clack, it will need a writable stream exposed
CC @florian-lefebvre — maybe relevant to your CLI refactors?
@43081j what API are you thinking? Something like a logger.stream
that Clack can use? I don’t know how crucial use of the logger is, but I guess it would be nice to keep using it for easy support of stuff like --silent
flags etc.
The confirm prompts are compared specifically against
true
(i.e.=== true
) since clack can return aSymbol
if the user cancels (ctrl-c). We need to decide if this is ok (same as behaviour in main), or we want toprocess.exit
or something
If behaviour is 1:1 with main
then I’m personally happy in terms of this PR and we can always improve on it later.
}), | ||
); | ||
spinner.success(); | ||
spinner.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick comparison of the two states here:
Current CLI | This PR |
---|---|
![]() |
![]() |
The “Resolving packages” message is preserved currently as a reference to the completed task, whereas it looks like Clack’s stop()
clears that out. I think it would be nice to preserve a message to refer back to.
Maybe as simple as?
spinner.stop(); | |
spinner.stop('Resolved packages'); |
} catch (e) { | ||
if (e instanceof Error) { | ||
spinner.error(e.message); | ||
spinner.stop(e.message, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking bombshell-dev/clack#405 here as a reminder to revisit if that change to the spinner API goes ahead.
const coloredOutput = `${bold(installCommand.command)} ${installCommand.args.join(' ')} ${cyan(packageNames.join(' '))}`; | ||
const message = `\n${boxen(coloredOutput, { | ||
margin: 0.5, | ||
padding: 0.5, | ||
borderStyle: 'round', | ||
})}\n`; | ||
logger.info( | ||
'SKIP_FORMAT', | ||
`\n ${magenta('Astro will run the following command:')}\n ${dim( | ||
'If you skip this step, you can always run it yourself later', | ||
)}\n${message}`, | ||
)}`, | ||
); | ||
clack.box(coloredOutput, undefined, { | ||
rounded: true, | ||
}); | ||
|
||
let response; | ||
if (options.skipAsk) { | ||
response = true; | ||
} else { | ||
response = ( | ||
await prompts({ | ||
type: 'confirm', | ||
name: 'askToContinue', | ||
response = | ||
(await clack.confirm({ | ||
message: 'Continue?', | ||
initial: true, | ||
}) | ||
).askToContinue; | ||
initialValue: true, | ||
})) === true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documenting the visual changes here:
Current CLI | This PR |
---|---|
![]() |
![]() |
Cancel state:
Current CLI | This PR |
---|---|
![]() |
![]() |
I quite like how the bold text in the old version helps the “Continue?” question stand out, so could be something for us to create a little set of styled utilities for? For example, something like this would help us use that style consistently:
const confirm = ({ message, ...opts }) =>
clack.confirm({ message: bold(message), ...opts });
As noted in my main comment, you can see the inconsistency of switching between the Clack style with the left border and the regular logging here.
if (!force) { | ||
const { shouldCopy } = await prompts({ | ||
type: 'confirm', | ||
name: 'shouldCopy', | ||
const shouldCopy = await clack.confirm({ | ||
message: 'Copy to clipboard?', | ||
initial: true, | ||
initialValue: true, | ||
}); | ||
|
||
if (!shouldCopy) return; | ||
if (shouldCopy !== true) return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, can you add a few justifications for the change to the description? Thank you. |
I've been watching this PR, thanks for the ping! I indeed wonder if it would be better to make it use the logger, how would that look in practice? |
Changes
This is a stacked PR - when we want to mark it as ready, there are 3 branches we can merge one after the other rather than this overall branch if we want to.
Basically, this changes the following 3 things to using clack:
TODO
Before we can land this, here's the remaining todo/open questions:
console
vsstdout
. If we want the logger to work with clack, it will need a writable stream exposedtrue
(i.e.=== true
) since clack can return aSymbol
if the user cancels (ctrl-c). We need to decide if this is ok (same as behaviour in main), or we want toprocess.exit
or somethingTesting
TBD
Docs
No docs need changing as this should provide the same DX.