Repository navigation
Conversation
4a9f57f to
7b99f2f
Compare
This fixes a couple of ergonomic regressions in Forge 8 when not using the tabbed logging UI: - We've been piping stdout/stderr from the app process any time `interactive` is true (i.e., any time `stdin` is a TTY). This makes code in the app process assume that it's running non-interactively, which will generally disable color output even when we're actually running in a terminal. - All log lines from the app have been prepended with an `[App]` tag. This is especially not ideal when not using the Webpack plugin, since no other plugins generate lines tagged with anything else. This commit makes it so that we only pipe stdout/stderr when we're actually using the tabbed UI, and otherwise we inherit stdio like we did before. This restores color output and removes the `[App]` tag from lines logged by the app, while still including tags on content logged by the Webpack plugin. You could argue that in the Webpack case it might be preferable to include the `[App]` tag, but I don't really think there's a good way to keep it -- I don't think it's worth the downsides of piping stdout and stderr naively, and I definitely don't think it's worth the extra complexity of using `node-pty` or something. One possibly controversial change here is that I had to remove the code in `logger.start()` that catches initialization errors from Ink and falls back to the simple logger. There's a chicken and egg problem here: - With the current sequencing, we've already spawned the app process by the time we call `logger.start()`, so it's too late to change its stdio mode. - We can't move the `logger.start()` call above the spawn call, because there are cases where spawning the app causes us to fall back to the simple logger. Unless there's some specific reason we expect Ink to sometimes throw at startup, the defensive code doesn't seem necessary. That said, if you do put a `throw` in the implementation of `startInk` the behavior is pretty ugly; this seems hard to fix, because you end up with a bunch of Webpack subprocesses printing errors on top of whatever error output we try to log. Note that although this is related to electron#4434, they each work independently and can be merged without conflicting.
7b99f2f to
702fe74
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (this.activeMode === 'ink') { | ||
| try { | ||
| const { startInk } = await import('./ink/render.js'); | ||
| if (this.hasStopped) return; | ||
| this.renderer = startInk({ | ||
| logger: this, | ||
| stdout: this.stdout, | ||
| stdin: this.stdin, | ||
| title: this.options.title, | ||
| keys: this.options.keys ?? [], | ||
| initialTab: this.options.initialTab, | ||
| errorSwitchDebounceMs: this.options.errorSwitchDebounceMs, | ||
| onQuit: () => this.quit(), | ||
| }); | ||
| this.inkActive = true; | ||
| return; | ||
| } catch (err) { | ||
| d( | ||
| 'failed to start the interactive UI, falling back to plain output', | ||
| err, | ||
| ); | ||
| this.activeMode = 'plain'; | ||
| } | ||
| const { startInk } = await import('./ink/render.js'); | ||
| if (this.hasStopped) return; | ||
| this.renderer = startInk({ | ||
| logger: this, | ||
| stdout: this.stdout, | ||
| stdin: this.stdin, | ||
| title: this.options.title, | ||
| keys: this.options.keys ?? [], | ||
| initialTab: this.options.initialTab, | ||
| errorSwitchDebounceMs: this.options.errorSwitchDebounceMs, | ||
| onQuit: () => this.quit(), | ||
| }); | ||
| this.inkActive = true; |
There was a problem hiding this comment.
🔴 When the terminal UI fails to load, electron-forge start now rejects after the Electron app is already running, instead of falling back to plain output as the base does. In Logger.start(), a rejected import('./ink/render.js') or a throwing startInk now escapes. started is already true and no renderer is set, so buffered tab output is only replayed at process exit. Fix: catch Ink load/start failures in start() and still render something. For example, fall back to PlainRenderer and pipe the already-piped child output into it, or print the error and stop the spawned app, so a load failure never leaves the app orphaned.
Why this was flagged
The trigger is any environment where the dynamic import of ./ink/render.js or the startInk call at Logger.ts:256-266 throws, such as a broken or missing ink/yoga install or an odd terminal. The PR description itself admits this case is "ugly". start.ts:462 calls forgeSpawnWrapper() first, which spawns Electron with stdio ['inherit','pipe','pipe'] because logger.mode is 'ink'. Then start.ts:466 awaits logger.start(), and it now rejects. The start API throws while the app child is still running. Its piped stdout/stderr are attached to tabs that will never render, and the readsRestartCommand/stdin setup is skipped. Webpack subprocesses also keep printing over the error. On the base, the try/catch logged the error, set activeMode='plain' and started a PlainRenderer, so the user still got output and the app kept working. Nothing else catches it: Logger.start has no other try/catch, and the spec that covered the fallback was deleted.
Verification: At the base commit, Logger.start() wrapped both calls in try/catch and fell back to a PlainRenderer. After this change, Logger.ts:255-268 has no catch, so the error escapes start() after Logger.ts:253 sets this.started = true. Electron is already spawned at start.ts:461, so await logger.start() (line 465) rejects while the child is running, and electron-forge-start.ts:81 has no catch.
| // that the App tab can show them (stdin stays inherited so the app can | ||
| // be interacted with). Otherwise the app inherits our real stdio and | ||
| // will see accurate `isTTY` values. | ||
| stdio: logger.mode === 'ink' ? ['inherit', 'pipe', 'pipe'] : 'inherit', |
There was a problem hiding this comment.
🟡 (optional) In some setups a programmatic start({ interactive: false }) call now pipes the app's stdout/stderr, nothing ever reads them, and the app can stall. This happens when the shared logger already exists in 'ink' mode. Since this PR, stdio depends on logger.mode === 'ink', but attachApp (start.ts:229) still returns early when !interactive. Fix: base the pipe decision and attachApp's early return on one condition, for example both on logger.mode === 'ink'. That way a piped child is always attached.
Why this was flagged
I could not rule out one population: hosts that call ensureSharedLogger() before api.start({ interactive: false }) in a TTY. A test runner or a wrapper script would do this. ensureSharedLogger (shared.ts) returns the existing logger and ignores interactive: false. That logger's mode was auto-detected as 'ink' (Logger.ts:69-74). start.ts:299 then spawns with ['inherit','pipe','pipe'], but attachApp at start.ts:229 returns because interactive is false, so nothing ever reads the pipes. Once the app writes about 64KB, its writes block and it hangs, and none of its output is shown. On the base branch, stdio used the same interactive flag as attachApp, so a non-interactive start always inherited stdio and the app's output went straight to the terminal.
Verification: nit. A host creates the shared logger in a TTY, then calls api.start({ interactive: false }). start.ts:299 now pipes stdio when logger.mode === 'ink', but attachApp returns first at start.ts:230, so nothing reads the pipes. On the base, line 299 was stdio: interactive ? ['inherit','pipe','pipe'] : 'inherit', so this path inherited stdio and could not stall.
The changes are appropriately documented (if applicable).Summarize your changes:
This fixes a couple of ergonomic regressions in Forge 8 when not using the tabbed logging UI, as mentioned in #4432:
We've been piping stdout/stderr from the app process any time
interactiveis true (i.e., any timestdinis a TTY). This makes code in the app process assume that it's running non-interactively, which will generally disable color output even when we're actually running in a terminal.All log lines from the app have been prepended with an
[App]tag. This is especially not ideal when not using the Webpack plugin, since there are no lines tagged with anything else.This commit makes it so that we only pipe stdout/stderr when we're actually using the tabbed UI, and otherwise we inherit stdio like we did before. This brings back color output and removes the
[App]tag from lines logged by the app, while still including tags on lines logged by the Webpack plugin.You could argue that in the Webpack case it might be better to include the
[App]tag, but I don't see a good way to keep it—I don't think it's worth the downsides of piping stdout/stderr, and I definitely don't think it's worth the extra complexity of usingnode-ptyor something.One possibly-controversial change is that I had to remove the code in
logger.start()that catches initialization errors from Ink and falls back to the simple logger. There's a chicken and egg problem here:With the current ordering, we've already spawned the app process by the time we call
logger.start(), so it's too late to change its stdio mode.We can't move the
logger.start()call above the spawn call, because there are cases where spawning the app causes us to fall back to the simple logger.Unless there's some specific reason we expect Ink to sometimes throw at startup, the defensive code doesn't seem necessary. That said, if you do hack
startInkto throw, the behavior is ugly; this seems hard to fix, because you end up with a bunch of Webpack subprocesses printing errors on top of whatever error output we try to log.