Skip to content

Commit 877321e

Browse files
Stop assuming CLI stdout is a TTY write stream (#51)
## Motivation for the change, related issues 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 We have since worked around the bug in the private Playground fork by using the same functions from the `readline` module, but this is also incorrect and can lead to some strange behavior. For example, sometimes two status messages are printed on the same line. If this fix lands in our private fork, perhaps we could port it to the public Playground repo so folks are not blocked from using Playground CLI. ## 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 [my PR to upgrade nx and vite](Automattic/wordpress-playground-private#35), `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 `blooprint.json` in that directory - 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=blooprint.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=blooprint.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.
1 parent 6911327 commit 877321e

File tree

2 files changed

+60
-22
lines changed

2 files changed

+60
-22
lines changed

packages/playground/cli/project.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@
3232
"dev": {
3333
"executor": "nx:run-commands",
3434
"options": {
35-
"command": "bun --watch ./packages/playground/cli/src/cli.ts"
35+
"command": "bun --watch ./packages/playground/cli/src/cli.ts",
36+
"tty": true
3637
}
3738
},
3839
"start": {

packages/playground/cli/src/run-cli.ts

Lines changed: 58 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import {
2121
import fs from 'fs';
2222
import { Server } from 'http';
2323
import path from 'path';
24-
import readline from 'readline';
2524
import { rootCertificates } from 'tls';
2625
import {
2726
CACHE_FOLDER,
@@ -127,29 +126,58 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer> {
127126

128127
const tracker = new ProgressTracker();
129128
let lastCaption = '';
130-
let progress100 = false;
129+
let progressReached100 = false;
131130
tracker.addEventListener('progress', (e: any) => {
132-
if (progress100) {
131+
if (progressReached100) {
133132
return;
134-
} else if (e.detail.progress === 100) {
135-
progress100 = true;
136133
}
134+
progressReached100 = e.detail.progress === 100;
135+
136+
// Use floor() so we don't report 100% until truly there.
137+
const progressInteger = Math.floor(e.detail.progress);
137138
lastCaption =
138139
e.detail.caption || lastCaption || 'Running the Blueprint';
139-
readline.clearLine(process.stdout, 0);
140-
readline.cursorTo(process.stdout, 0);
141-
process.stdout.write(
142-
'\r\x1b[K' + `${lastCaption.trim()}${e.detail.progress}%`
143-
);
144-
if (progress100) {
145-
process.stdout.write('\n');
140+
const message = `${lastCaption.trim()}${progressInteger}%`;
141+
if (!args.quiet) {
142+
writeProgressUpdate(
143+
process.stdout,
144+
message,
145+
progressReached100
146+
);
146147
}
147148
});
148149
return compileBlueprint(blueprint as Blueprint, {
149150
progress: tracker,
150151
});
151152
}
152153

154+
let lastProgressMessage = '';
155+
function writeProgressUpdate(
156+
writeStream: NodeJS.WriteStream,
157+
message: string,
158+
finalUpdate: boolean
159+
) {
160+
if (message === lastProgressMessage) {
161+
// Avoid repeating the same message
162+
return;
163+
}
164+
lastProgressMessage = message;
165+
166+
if (writeStream.isTTY) {
167+
// Overwrite previous progress updates in-place for a quieter UX.
168+
writeStream.cursorTo(0);
169+
writeStream.write(message);
170+
writeStream.clearLine(1);
171+
172+
if (finalUpdate) {
173+
writeStream.write('\n');
174+
}
175+
} else {
176+
// Fall back to writing one line per progress update
177+
writeStream.write(`${message}\n`);
178+
}
179+
}
180+
153181
if (args.quiet) {
154182
// @ts-ignore
155183
logger.handlers = [];
@@ -169,23 +197,32 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer> {
169197

170198
logger.log(`Setting up WordPress ${args.wp}`);
171199
let wpDetails: any = undefined;
200+
// @TODO: Rename to FetchProgressMonitor. There's nothing Emscripten
201+
// about that class anymore.
172202
const monitor = new EmscriptenDownloadMonitor();
173203
if (!args.skipWordPressSetup) {
174-
// @TODO: Rename to FetchProgressMonitor. There's nothing Emscripten
175-
// about that class anymore.
204+
let progressReached100 = false;
176205
monitor.addEventListener('progress', ((
177206
e: CustomEvent<ProgressEvent & { finished: boolean }>
178207
) => {
179-
// @TODO Every progres bar will want percentages. The
208+
if (progressReached100) {
209+
return;
210+
}
211+
212+
// @TODO Every progress bar will want percentages. The
180213
// download monitor should just provide that.
181-
const percentProgress = Math.round(
182-
Math.min(100, (100 * e.detail.loaded) / e.detail.total)
214+
const { loaded, total } = e.detail;
215+
// Use floor() so we don't report 100% until truly there.
216+
const percentProgress = Math.floor(
217+
Math.min(100, (100 * loaded) / total)
183218
);
219+
progressReached100 = percentProgress === 100;
220+
184221
if (!args.quiet) {
185-
readline.clearLine(process.stdout, 0);
186-
readline.cursorTo(process.stdout, 0);
187-
process.stdout.write(
188-
`Downloading WordPress ${percentProgress}%...`
222+
writeProgressUpdate(
223+
process.stdout,
224+
`Downloading WordPress ${percentProgress}%...`,
225+
progressReached100
189226
);
190227
}
191228
}) as any);

0 commit comments

Comments
 (0)