Skip to content

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Oct 20, 2025

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:

  • boxen (CLI boxes)
  • yocto-spinner (CLI spinner)
  • prompts (CLI prompts, only used for confirm prompt)

TODO

Before we can land this, here's the remaining todo/open questions:

  • 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)
  • 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 vs stdout. If we want the logger to work with clack, it will need a writable stream exposed
  • The confirm prompts are compared specifically against true (i.e. === true) since clack can return a Symbol if the user cancels (ctrl-c). We need to decide if this is ok (same as behaviour in main), or we want to process.exit or something

Testing

TBD

Docs

No docs need changing as this should provide the same DX.

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.
Copy link

changeset-bot bot commented Oct 20, 2025

⚠️ No Changeset found

Latest commit: d82cfa1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 20, 2025
Copy link

codspeed-hq bot commented Oct 20, 2025

CodSpeed Performance Report

Merging #14589 will not alter performance

Comparing 43081j:clack-prompts (d82cfa1) with main (7958c6b)1

Summary

✅ 6 untouched

Footnotes

  1. No successful run was found on main (ca1d331) during the generation of this report, so 7958c6b was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@delucis
Copy link
Member

delucis commented Oct 21, 2025

Noting before I forget that a good follow-up/companion to this would be replacing kleur with picocolors in our code. This PR removes transitive use of kleur, yoctocolors, and chalk, and picocolors is pulled in by vite via postcss, so we could drop another package by switching.

Copy link
Member

@delucis delucis left a 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 vs stdout. 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 a Symbol if the user cancels (ctrl-c). We need to decide if this is ok (same as behaviour in main), or we want to process.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();
Copy link
Member

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
Checkmark icon Resolving packages... diamond icon with no text

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?

Suggested change
spinner.stop();
spinner.stop('Resolved packages');

} catch (e) {
if (e instanceof Error) {
spinner.error(e.message);
spinner.stop(e.message, 2);
Copy link
Member

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.

Comment on lines -78 to 95
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;
}
Copy link
Member

@delucis delucis Oct 21, 2025

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
image image

Cancel state:

Current CLI This PR
image image

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.

Comment on lines 126 to 133
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;
}
Copy link
Member

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
image image
image image

Looks good I think — best not to add borders to the list logged here to make manual copy/paste easy. Could bold the question perhaps, as noted above?

@matthewp
Copy link
Contributor

Hello, can you add a few justifications for the change to the description? Thank you.

@florian-lefebvre
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants