Skip to content

Commit

Permalink
Stop assuming CLI stdout is a TTY write stream (#2154)
Browse files Browse the repository at this point in the history
## Motivation for the change, related issues

Related to #2127

In WordPress/wordpress-playground, we still assume that the Playground
CLI is running in a TTY context, and the CLI breaks when not in a TTY
context [because `process.stdout.clearLine()` and
`process.stdout.clearLine()` are
missing](#2127).

Here are some lines that fail without a TTY context:


https://github.com/WordPress/wordpress-playground/blob/f5b85399ff8dbabaf122bfa98347ce76d7f59910/packages/playground/cli/src/cli.ts#L232-L233

## Implementation details

This PR does a number of things:
- Updates the progress messages to be written in-place over one another
when in TTY contexts
- Updates the progress messages to be written on separate lines when in
non-TTY contexts
- Makes sure that identical progress messages are not repeated across
multiple lines in non-TTY mode
- Ensures that messages following in-place progress updates are written
on the next line (sometimes two messages were appearing on the same line
like "Downloading WordPress 100%... Booted!")
- Makes sure the progress percentage is an integer and doesn't report
100% until truly at 100%.
- Explicitly declares that the nx `run-commands` executor for
`playground-cli:dev` should run in TTY mode.
- It doesn't seem to affect our current version of nx, but when testing
with a WIP branch to upgrade nx and vite, `npx nx dev playground-cli`
did seem to run in a TTY context.
- Either way [the nx tty
option](https://nx.dev/nx-api/nx/executors/run-commands#tty), doesn't
seem to adversely affect anything, so I'm leaving it explicitly declared
that way for later.

## Testing Instructions (or ideally a Blueprint)

- `cd` into your Playground working dir
- Create a blueprint named `test-blueprint.json` in that directory based
on [this
gist](https://gist.github.com/brandonpayton/70e7b73e6dd8b87053db691db70b42dc#file-blueprint-json).
- Test in TTY mode
- Pick a WP version Playground hasn't downloaded or simply run `rm -r
~/.wordpress-playground` to clear the Playground CLI cache.
- Run `npx bun --watch ./packages/playground/cli/src/cli.ts server
--blueprint=test-blueprint.json`
- Observe that the Download and Blueprint progress messages are updated
on a single line each as they progress to 100%
   - Observe that each message appears on its own line
- Test in non-TTY mode
- Pick a WP version Playground hasn't downloaded or simply run `rm -r
~/.wordpress-playground` to clear the Playground CLI cache.
- Run `npx bun --watch ./packages/playground/cli/src/cli.ts server
--blueprint=test-blueprint.json | more`
    - Piping output to `more` causes the CLI's stdout to not be a TTY
- Hit the spacebar to proceed through the output (this may be obvious,
but sometimes it took me a second or two to remember :)
- Observe that each Download and Blueprint progress message is printed
on a new line as they progress to 100%.
  - Observe that no progress messages are repeated.
  • Loading branch information
brandonpayton authored Feb 22, 2025
1 parent 1a8e521 commit 18bd27a
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 21 deletions.
3 changes: 2 additions & 1 deletion packages/playground/cli/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
"dev": {
"executor": "nx:run-commands",
"options": {
"command": "bun --watch ./packages/playground/cli/src/cli.ts"
"command": "bun --watch ./packages/playground/cli/src/cli.ts",
"tty": true
}
},
"start": {
Expand Down
78 changes: 58 additions & 20 deletions packages/playground/cli/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,29 +220,58 @@ async function run() {

const tracker = new ProgressTracker();
let lastCaption = '';
let progress100 = false;
let progressReached100 = false;
tracker.addEventListener('progress', (e: any) => {
if (progress100) {
if (progressReached100) {
return;
} else if (e.detail.progress === 100) {
progress100 = true;
}
progressReached100 = e.detail.progress === 100;

// Use floor() so we don't report 100% until truly there.
const progressInteger = Math.floor(e.detail.progress);
lastCaption =
e.detail.caption || lastCaption || 'Running the Blueprint';
process.stdout.clearLine(0);
process.stdout.cursorTo(0);
process.stdout.write(
'\r\x1b[K' + `${lastCaption.trim()}${e.detail.progress}%`
);
if (progress100) {
process.stdout.write('\n');
const message = `${lastCaption.trim()}${progressInteger}%`;
if (!args.quiet) {
writeProgressUpdate(
process.stdout,
message,
progressReached100
);
}
});
return compileBlueprint(blueprint as Blueprint, {
progress: tracker,
});
}

let lastProgressMessage = '';
function writeProgressUpdate(
writeStream: NodeJS.WriteStream,
message: string,
finalUpdate: boolean
) {
if (message === lastProgressMessage) {
// Avoid repeating the same message
return;
}
lastProgressMessage = message;

if (writeStream.isTTY) {
// Overwrite previous progress updates in-place for a quieter UX.
writeStream.cursorTo(0);
writeStream.write(message);
writeStream.clearLine(1);

if (finalUpdate) {
writeStream.write('\n');
}
} else {
// Fall back to writing one line per progress update
writeStream.write(`${message}\n`);
}
}

const command = args._[0] as string;
if (!['run-blueprint', 'server', 'build-snapshot'].includes(command)) {
yargsObject.showHelp();
Expand All @@ -263,23 +292,32 @@ async function run() {

logger.log(`Setting up WordPress ${args.wp}`);
let wpDetails: any = undefined;
// @TODO: Rename to FetchProgressMonitor. There's nothing Emscripten
// about that class anymore.
const monitor = new EmscriptenDownloadMonitor();
if (!args.skipWordPressSetup) {
// @TODO: Rename to FetchProgressMonitor. There's nothing Emscripten
// about that class anymore.
let progressReached100 = false;
monitor.addEventListener('progress', ((
e: CustomEvent<ProgressEvent & { finished: boolean }>
) => {
// @TODO Every progres bar will want percentages. The
if (progressReached100) {
return;
}

// @TODO Every progress bar will want percentages. The
// download monitor should just provide that.
const percentProgress = Math.round(
Math.min(100, (100 * e.detail.loaded) / e.detail.total)
const { loaded, total } = e.detail;
// Use floor() so we don't report 100% until truly there.
const percentProgress = Math.floor(
Math.min(100, (100 * loaded) / total)
);
progressReached100 = percentProgress === 100;

if (!args.quiet) {
process.stdout.clearLine(0);
process.stdout.cursorTo(0);
process.stdout.write(
`Downloading WordPress ${percentProgress}%...`
writeProgressUpdate(
process.stdout,
`Downloading WordPress ${percentProgress}%...`,
progressReached100
);
}
}) as any);
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 18bd27a

Please sign in to comment.