stackblitz / webcontainer-core

Dev environments. In your web app.
https://webcontainers.io
MIT License
3.79k stars 145 forks source link

Differentiate between `stdout` and `stderr` #971

Open Rich-Harris opened 1 year ago

Rich-Harris commented 1 year ago

Is your feature request related to a problem? Please describe:

In pre-release versions of the WebContainer API, it was possible to pass stdout and stderr callbacks to a process created with vm.run:

const process = await instance.run({ command, args }, {
  stdout: (data) => console.log(data),
  stderr: (data) => console.error(data)
});

As of 1.0, process.output is a ReadableStream containing both stdout and stderr combined:

const process = await instance.spawn(command, args);
process.output.pipeTo(new WritableStream({
  write(chunk) {
    console.log(chunk);
  }
}));

In general this is a much nicer API! Sometimes it's useful to know which is which though — for example on learn.svelte.dev we could use stderr messages to set a flag that would cause the iframe to reload.

Describe the solution you'd like:

process.stdout.pipeTo(...);
process.stderr.pipeTo(...);

Describe alternatives you've considered:

I wondered about this alternative, but I'm not sure it has any advantages (and is obviously a breaking change):

process.output.pipeTo(new WritableStream({
  write(chunk) {
+    if (chunk.type === 'stdout') {
+      console.log(chunk);
+    } else if (chunk.type === 'stderr') {
+      console.error(chunk);
+    }
  }
}));
jrvidal commented 1 year ago

:wave: Thanks for the report.

FWIW, the rationale for the change is that spawn() is akin to forkpty (or spawn in node-pty). Once there is a pseudoterminal mediating between the "parent" (the embedder, learn.svelte.dev in this case) and the child, it doesn't make a lot of sense to distinguish between stdout and stderr (consider for instance that you can echo foobar > /dev/tty).

Sometimes it's useful to know which is which though — for example on learn.svelte.dev we could use stderr messages to set a flag that would cause the iframe to reload.

When you say "we could use", do you mean it is the way things are set up now that you're using the 0.x version? Or something that you were considering to add?

Could you elaborate a bit more on what you want to achieve? Maybe we'll find a more natural set of capabilities we could add to serve your use case.

Rich-Harris commented 1 year ago

When you say "we could use"

When we were using 0.x, we would set a vite_error boolean to true (https://github.com/sveltejs/learn.svelte.dev/pull/114) in the case where stderr was emitted, since this typically indicated that the app was in a broken state that could only be recovered from by reloading the page. Since 1.0, that's not possible, so the user will have to figure out that they need to manually reload

jrvidal commented 1 year ago

We're considering adding a different mode for spawn() that would bypass the creation of a TTY. The returned process would have stdin, stdout and stderr streams (somewhat similar to stdio: 'pipe' for a Node child process) instead of input/output.

The "downside" is that, without a TTY, this mode would not be suitable to run terminal-based applications e.g. you could not spawn a shell and feed it keystrokes. But it sounds that it would be good enough for http://learn.svelte.dev. WDYT?

Rich-Harris commented 1 year ago

Yeah, that would work for us! We don't need an interactive terminal, so if losing that ability is the price of being able to differentiate between stdout and stderr then I think that makes sense