Skip to content

Commit

Permalink
fix(node): support passing parent stdio streams (denoland#19171)
Browse files Browse the repository at this point in the history
This is a bit bare bones but gets `npm-run-all` working. For full stdio
compatibility with node more work is needed which is probably better
done in follow up PRs.

Fixes denoland#19159
  • Loading branch information
marvinhagemeister authored May 18, 2023
1 parent 9dc3ae8 commit 695b5de
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 10 deletions.
22 changes: 22 additions & 0 deletions cli/tests/unit_node/child_process_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -577,3 +577,25 @@ Deno.test(
assertStringIncludes(output, "typescript");
},
);

Deno.test(
"[node/child_process spawn] supports stdio array option",
async () => {
const cmdFinished = deferred();
let output = "";
const script = path.join(
path.dirname(path.fromFileUrl(import.meta.url)),
"testdata",
"child_process_stdio.js",
);
const cp = spawn(Deno.execPath(), ["run", "-A", script]);
cp.stdout?.on("data", (data) => {
output += data;
});
cp.on("close", () => cmdFinished.resolve());
await cmdFinished;

assertStringIncludes(output, "foo");
assertStringIncludes(output, "close");
},
);
15 changes: 15 additions & 0 deletions cli/tests/unit_node/testdata/child_process_stdio.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import childProcess from "node:child_process";
import process from "node:process";
import * as path from "node:path";

const script = path.join(
path.dirname(path.fromFileUrl(import.meta.url)),
"node_modules",
"foo",
"index.js",
);

const child = childProcess.spawn(process.execPath, [script], {
stdio: [process.stdin, process.stdout, process.stderr],
});
child.on("close", () => console.log("close"));
50 changes: 40 additions & 10 deletions ext/node/polyfills/internal/child_process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,9 @@ export class ChildProcess extends EventEmitter {
args: cmdArgs,
cwd,
env: stringEnv,
stdin: toDenoStdio(stdin as NodeStdio | number),
stdout: toDenoStdio(stdout as NodeStdio | number),
stderr: toDenoStdio(stderr as NodeStdio | number),
stdin: toDenoStdio(stdin),
stdout: toDenoStdio(stdout),
stderr: toDenoStdio(stderr),
windowsRawArguments: windowsVerbatimArguments,
}).spawn();
this.pid = this.#process.pid;
Expand All @@ -189,6 +189,16 @@ export class ChildProcess extends EventEmitter {
this.stdin = Writable.fromWeb(this.#process.stdin);
}

if (stdin instanceof Stream) {
this.stdin = stdin;
}
if (stdout instanceof Stream) {
this.stdout = stdout;
}
if (stderr instanceof Stream) {
this.stderr = stderr;
}

if (stdout === "pipe") {
assert(this.#process.stdout);
this.stdout = Readable.fromWeb(this.#process.stdout);
Expand Down Expand Up @@ -285,15 +295,22 @@ export class ChildProcess extends EventEmitter {

async #_waitForChildStreamsToClose() {
const promises = [] as Array<Promise<void>>;
if (this.stdin && !this.stdin.destroyed) {
// Don't close parent process stdin if that's passed through
if (this.stdin && !this.stdin.destroyed && this.stdin !== process.stdin) {
assert(this.stdin);
this.stdin.destroy();
promises.push(waitForStreamToClose(this.stdin));
}
if (this.stdout && !this.stdout.destroyed) {
// Only readable streams need to be closed
if (
this.stdout && !this.stdout.destroyed && this.stdout instanceof Readable
) {
promises.push(waitForReadableToClose(this.stdout));
}
if (this.stderr && !this.stderr.destroyed) {
// Only readable streams need to be closed
if (
this.stderr && !this.stderr.destroyed && this.stderr instanceof Readable
) {
promises.push(waitForReadableToClose(this.stderr));
}
await Promise.all(promises);
Expand All @@ -317,9 +334,13 @@ const supportedNodeStdioTypes: NodeStdio[] = ["pipe", "ignore", "inherit"];
function toDenoStdio(
pipe: NodeStdio | number | Stream | null | undefined,
): DenoStdio {
if (pipe instanceof Stream) {
return "inherit";
}

if (
!supportedNodeStdioTypes.includes(pipe as NodeStdio) ||
typeof pipe === "number" || pipe instanceof Stream
typeof pipe === "number"
) {
notImplemented(`toDenoStdio pipe=${typeof pipe} (${pipe})`);
}
Expand Down Expand Up @@ -441,8 +462,17 @@ function normalizeStdioOption(
"pipe",
"pipe",
],
) {
): [
Stream | NodeStdio | number,
Stream | NodeStdio | number,
Stream | NodeStdio | number,
...Array<Stream | NodeStdio | number>,
] {
if (Array.isArray(stdio)) {
// At least 3 stdio must be created to match node
while (stdio.length < 3) {
ArrayPrototypePush(stdio, undefined);
}
return stdio;
} else {
switch (stdio) {
Expand Down Expand Up @@ -796,8 +826,8 @@ export function spawnSync(
args,
cwd,
env,
stdout: toDenoStdio(normalizedStdio[1] as NodeStdio | number),
stderr: toDenoStdio(normalizedStdio[2] as NodeStdio | number),
stdout: toDenoStdio(normalizedStdio[1]),
stderr: toDenoStdio(normalizedStdio[2]),
uid,
gid,
windowsRawArguments: windowsVerbatimArguments,
Expand Down

0 comments on commit 695b5de

Please sign in to comment.