Skip to content

Keep Terminal logs in order with other stdout/stderr output - #2162

Merged
vzaidman merged 7 commits into
react:mainfrom
vzaidman:terminal-output-order
Nov 4, 2026
Merged

vzaidman merged 7 commits into
react:mainfrom
vzaidman:terminal-output-order

Conversation

@vzaidman

@vzaidman vzaidman commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Summary

In interactive mode, Terminal.log() queued lines and wrote them with the next status update. When an update was already in flight, the line was written up to ~33ms later, after any console.log, console.error or other direct writes made in the meantime, so output appeared out of order.

  • log() writes the line immediately: it discards the written status (erasing it unless other output followed it), writes the line, then schedules the status redraw. Status redraws remain throttled, so a burst of logs redraws the status once.
  • persistStatus() clears the pending status before logging it, so the redraw doesn't draw it again.
  • Status updates call readline.moveCursor, readline.clearScreenDown and stream.write directly instead of promisify wrappers. An update resolves on its last write's callback, as writes to a stream complete in order. Write errors no longer reject flush() / waitForUpdates().
  • Private members are renamed for clarity (e.g. #nextStatusStr -> #status, #statusStr -> #writtenStatus). The public API is unchanged.

Changelog: [Fix] Keep Terminal.log() output in order with other stdout/stderr output

Test plan

  • New test "keeps logs in order with output written outside of Terminal" fails before this change and passes after.
  • yarn jest packages/metro-core packages/metro/src/lib
  • yarn flow check, yarn eslint packages/metro-core/src

Summary:
In interactive mode, `Terminal.log()` queued lines and wrote them with the next status update. When an update was already in flight, the line was written up to ~33ms later, after any `console.log`, `console.error` or other direct writes made in the meantime, so output appeared out of order.

- `log()` writes the line immediately: it discards the written status (erasing it unless other output followed it), writes the line, then schedules the status redraw. Status redraws remain throttled, so a burst of logs redraws the status once.
- `persistStatus()` clears the pending status before logging it, so the redraw doesn't draw it again.
- Status updates call `readline.moveCursor`, `readline.clearScreenDown` and `stream.write` directly instead of `promisify` wrappers. An update resolves on its last write's callback, as writes to a stream complete in order. Write errors no longer reject `flush()` / `waitForUpdates()`.
- Private members are renamed for clarity (e.g. `#nextStatusStr` -> `#status`, `#statusStr` -> `#writtenStatus`). The public API is unchanged.

Changelog: [Fix] Keep `Terminal.log()` output in order with other stdout/stderr output

Test Plan:
- New test "keeps logs in order with output written outside of Terminal" fails before this change and passes after.
- `yarn jest packages/metro-core packages/metro/src/lib`
- `yarn flow check`, `yarn eslint packages/metro-core/src`
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 3, 2026
@vzaidman
vzaidman requested review from huntie and robhogan November 3, 2026 15:49
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Metro review: Looks good

Risk: low · Commit: 47808a7 · 10 commands run

Terminal.log() now writes immediately instead of queuing lines for the next status update, and status redraws stay throttled. Run against both checkouts, the PR's ordering tests fail on base and pass on head, and raw terminal sequences show log1, external output, log2 in order on head where base put the external output first. The earlier waitForUpdates() finding is fixed at this head, and I found no new problems.

No findings.

Verified
  • ✅ yarn jest --no-watchman packages/metro-core — 36 tests pass at head, including the new Terminal tests.
  • ✅ yarn jest --no-watchman packages/metro/src/lib — 174 tests pass at head.
  • ✅ yarn eslint packages/metro-core/src/Terminal.js packages/metro-core/src/__tests__/Terminal-test.js
  • ✅ yarn flow check — No errors.
  • ✅ yarn verify-api-snapshots — 18 snapshots verified.
  • ✅ yarn --cwd <base-worktree> jest --no-watchman --forceExit packages/metro-core/src/__tests__/TerminalPr-test.js (failed as expected) — Head's Terminal-test.js run against base: 6 of 27 fail, including both ordering tests, persistStatus and the flush/waitForUpdates write-wait tests.
  • ✅ yarn jest --no-watchman --forceExit packages/metro-core/src/__tests__/TerminalRaw-test.js (head and base) — Real node:readline over a fake TTY. Head emits log1, external, log2, status; base emits external before the logs. Idle burst, persist and multi-line differ only in that head writes each line separately.
  • ❌ yarn jest --no-watchman packages/metro-core/src/__tests__/TerminalReview-test.js (head) — Errored-stream and empty-write cases pass. The undrained non-TTY PassThrough case fails because flush() waits for the drain, which matches the PR's stated flush semantics, so it is not reported as a bug.
  • ✅ yarn --cwd <base-worktree> jest --no-watchman --forceExit packages/metro-core/src/__tests__/TerminalReview-test.js (failed as expected) — On base, a non-TTY write error crashes the process with an unhandled rejection from log(). Head settles, as the PR describes. This crash stopped the base run, so the undrained case has no separate base result.
  • ⚠️ python3 -I repro/run-pty.py — Real pty comparison could not start: forkpty failed with 'out of pty devices' under the sandbox.

Not run: No real terminal: pty allocation failed in the sandbox, so sequences were checked on a fake TTY with real node:readline, and rendering was not observed. Windows untested (no machine available), including async TTY writes and bytesWritten detection. No benchmark: no performance claim and not a per-module hot path, though log bursts now make one write per line instead of a batch (cost unmeasured). A shell command combining cd with writes was denied by the permission system; its checks were rerun without cd.

Automated review by Claude. Advisory only: it never approves or blocks, and a maintainer makes the call.

Comment thread packages/metro-core/src/Terminal.js Resolved
Comment thread packages/metro-core/src/Terminal.js Resolved
Comment thread packages/metro-core/src/Terminal.js Resolved

@robhogan robhogan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to make sure this isn't isn't perf regressing (especially for main thread dominated builds) since stdout can be sneaky like that - but it looks fine:

https://robhogan.github.io/reports/metro-terminal-output/

@vzaidman
vzaidman merged commit 62f28a2 into react:main Nov 4, 2026
16 checks passed
@vzaidman

vzaidman commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

I wanted to make sure this isn't isn't perf regressing (especially for main thread dominated builds) since stdout can be sneaky like that - but it looks fine:

https://robhogan.github.io/reports/metro-terminal-output/

Cheers!

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants