Add .node() method

[GMartigny]

Fix #65

[GMartigny]

This is a working draft to check if I'm going the right way.

As per #65 (comment), the fork method is just doing a spawn of a node process with some options.

[ehmicky]

Thanks this new PR @GMartigny!

This is going the right direction except:

• you should execa(execPath, ...) not execa('node', ..., { execPath })
• changing stdio.js like this won't work because it's going to add this fourth channel to other methods as well, not only to fork().
• there are still few things missing (please check the issue, I think I've listed them). Please let me know once you've got those covered as well!

[GMartigny]

Ok thanks @ehmicky for the help.

changing stdio.js like this won't work because it's going to add this fourth channel to other methods as well

Is that a problem if that forth channel is only ever set to undefined on other cases ?

use process.execPath ...

process.execPath should be the running file, execa main file or something else ?

use process.execArgv ...

Should we redirect all node arguments to this option ?

[ehmicky]

Is that a problem if that forth channel is only ever set to undefined on other cases ?

Unless I am mistaken, this seems like at the moment, stdio: "inherit" would translate to stdio: ["inherit", "inherit", "inherit", "ipc"] for both fork() (which is good) and normal execa() (which is not wanted).

This could be solved by doing the stdio normalization logic before calling execa() inside fork().

process.execPath should be the running file, execa main file or something else ?

Yes process.execPath is the currently run node binary. I.e. it is the main file spawned.

Should we redirect all node arguments to this option ?

We need to pass process.execArgv down to the child process. Now the core Node.js fork() method also allows overidding with opts.execArgv. Users might expect the same options as the core Node.js method, i.e. it might be a good idea to add this.

[GMartigny]

From what I read on the node's source code, you can call fork with one 'ipc' in any channel of stdio. Should we comply with this behavior ? This would add more complexity to options.

Also, there are (IMO) arbitrary choices over options hierarchy. For example, an array stdio ignores the silent option.

Here's what make sens to me, but I need confirmation:

options => effective result

1 {}                      => ['inherit', 'inherit', 'inherit', 'ipc']    // default to 'inherit'
2 {stdio: 'pipe'}         => ['pipe', 'pipe', 'pipe', 'ipc']             // Can be defined
3 {stdio: 'whatever'}     => ['whatever', 'whatever', 'whatever', 'ipc'] // Let user fail
4 {stdio: [0, 1, 2]}      => [0, 1, 2, 'ipc']                            // Keep stdio arrays as is
4b {stdio: [0, 1, 2, 3]}  => [0, 1, 2, 'ipc']                            // But ignores overflow
5 {stdio: [0, 'ipc', 2]}  => [0, 'ipc', 2]                               // According to node's code

6 {stdout: 'any'} => ['inherit', 'any', 'inherit', 'ipc'] // Can defined individual channel
7 {stdout: 'ipc'} => ['inherit', 'ipc', 'inherit']        // According to node's code

8 {stdio: 'any', silent: true}     => ['pipe', 'pipe', 'pipe', 'ipc'] // Silent take precedence
9 {stdio: [0, 1, 2], silent: true} => ['pipe', 'pipe', 'pipe', 'ipc'] // Silent take precedence
10 {stdout: 'any', silent: true}   => ['pipe', 'pipe', 'pipe', 'ipc'] // Silent take precedence

Lines 3, 4, 4b and 9 don't follow what I read from Node's doc or code.

[ehmicky]

All that logic is already handled by both execa and Node.js child_process module. The only thing that's needed here is IMHO, inside fork(), before calling main execa() method:

1. if only one string was passed, expand to an array of three repeated strings (already done by stdio.js, just need to call it and maybe tweak it)
2. if an array was passed, set array[3] to ipc

This is in essence what the Node.js code for fork() seems to do there as well.

The lines you describe do seem to follow Node.js core behavior for fork(), I think you might be missing the lines related to this logic. They do handle things in a stricter way because if the user passes an array for stdio, they do not normalize it. Instead they just throw if ipc is not present.

[GMartigny]

if an array was passed, set array[3] to ipc

We don't have to restrict the ipc channel to the 3rd one ? I don't know why, but why not let the user define it's stdio and pushing ipc to the last available position.

If I push Unit Tests for that new function, could you review them ? I feel it would be better going TDD style.

[ehmicky]

Sure any process works better for you will work for me :)

Yes you're right about ipc. For reference, this is the Node.js core code for this.

[ehmicky]

The tests look good 👍

[GMartigny]

Hi, tests are failing on Windows because:

// This will be run on a fork of Node, which can't work
execa.fork("fixtures/hello.cmd"); // => node fixtures/hello.cmd

// Will spawn a new cmd that don't exit itself
execa.fork("fixtures\\hello.cmd", [], {execPath: "cmd.exe"}); // => cmd.exe fixtures\hello.cmd

// Same as
execa("cmd.exe", ["fixtures\\hello.cmd"]);

I saw that node-cross-spawn add a bunch of options to cmd. But only on certain cases that we don't fulfill here.

Should we add the options ourselves or am I missing something ?

[ehmicky]

I don't think the fork() method is meant to fire shell scripts (neither cmd.exe nor Bash) but only Node.js files, since it uses node as the main binary file.

I.e. the two first commands above should throw an exception.

[ehmicky]

Thanks for going through this other round of reviews @GMartigny!

This looks good (except for small things).

However personally I think moving the tests and fixtures should have been done in a separate PR. But now that it's done, I guess we'll keep like that :)

[sindresorhus]

This is not correct. It's not just a file, it's a script, and it's not a forked process, it's "spawning Node.js". Forking a process is a completely different thing.

[ehmicky]

Related to #200 (comment)

[sindresorhus]

Only a few minor tweaks and this looks good to me :)

You also need to fix the merge conflict.

[sindresorhus]

Yay! Thanks for working on this :)