CLI: route picospinner output to stderr so --format=json stays pipeable - #3219
Conversation
picospinner hard-codes process.stdout.write in its Renderer, so spinner frames leaked into the JSON payload of commands like `site list --format=json`, breaking `| jq`. Wrap the shared renderer's render and onComponentFinish methods to temporarily redirect stdout writes to stderr. Regression introduced in #3035 (ora → picospinner) which shipped in 1.7.9. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📊 Performance Test ResultsComparing 44eade0 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
ea27f7c to
0648899
Compare
|
I already mentioned this on Slack, but to connect the dots: I think a better solution would be to silence the |
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
@nightnei had to persuade me that printing spinners to stderr is the right thing to do, but after seeing https://clig.dev/#the-basics, I was convinced 👍
This change is substantive enough that I think it almost warrants just switching back to ora… This works, though, so I'm fine with proceeding. Although I would encourage you to make an upstream contribution to picospinner, @nightnei. This could be a config property on the DisplayOptions object passed to the Spinner constructor.
As for the specifics of this PR, I suggest patching Spinner instead of renderer, to avoid touching private methods.
| @@ -1,3 +1,4 @@ | |||
| import 'cli/lib/picospinner-stderr-patch'; | |||
There was a problem hiding this comment.
Let's move this to apps/cli/logger.ts
| * Wrap the renderer's two write-emitting methods so any `process.stdout.write` | ||
| * calls they make are rerouted to stderr for the duration of the call. This | ||
| * avoids duplicating picospinner internals and survives upstream changes as | ||
| * long as `render` and `onComponentFinish` remain the only write paths. |
There was a problem hiding this comment.
If we make my suggested change, let's also update this comment.
Co-authored-by: Fredrik Rombach Ekelund <fredrik@f26d.dev>
|
@fredrikekelund JFYI I have created PR to picospinner - tinylibs/picospinner#13 |
3 participants
Related issues
Fixes STU-1637
Related to #3035 (ora → picospinner swap that introduced the regression)
How AI was used in this PR
Assisted with investigation and writing code. We had a few iterations, I thoroughly reviewed and tested it.
Proposed Changes
orawrites to stderr by default.picospinnerhard-codesprocess.stdout.write(...)inside itsRendererand exposes no option to change the stream. As a result, shipping wp-studio@1.7.9 started emitting spinner frames on stdout, alongside the JSON payload of commands likesite list --format=json— breaking| jqand any scripted consumer.Testing Instructions
npm run cli:buildnode apps/cli/dist/cli/main.mjs site list --format=json | jqjq: parse error: Invalid numeric literal at line 1, column 3)node apps/cli/dist/cli/main.mjs site list