Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .node() method #200

Merged
merged 47 commits into from Jun 18, 2019
Merged

Conversation

GMartigny
Copy link
Contributor

Fix #65

@GMartigny
Copy link
Contributor Author

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
Copy link
Collaborator

ehmicky commented Apr 3, 2019

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
Copy link
Contributor Author

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
Copy link
Collaborator

ehmicky commented Apr 3, 2019

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
Copy link
Contributor Author

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
Copy link
Collaborator

ehmicky commented Apr 3, 2019

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
Copy link
Contributor Author

GMartigny commented Apr 4, 2019

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
Copy link
Collaborator

ehmicky commented Apr 4, 2019

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.

test/stdio.js Show resolved Hide resolved
test/stdio.js Show resolved Hide resolved
test/stdio.js Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator

ehmicky commented Apr 4, 2019

The tests look good 👍

lib/stdio.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
fixtures/plain.js Outdated Show resolved Hide resolved
@GMartigny
Copy link
Contributor Author

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
Copy link
Collaborator

ehmicky commented Apr 8, 2019

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.

@GMartigny GMartigny changed the title Add fork method Add .node() method Jun 6, 2019
Copy link
Collaborator

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

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 :)

readme.md Outdated Show resolved Hide resolved
test/fixtures/send.js Outdated Show resolved Hide resolved
test/node.js Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated
@@ -180,6 +180,15 @@ Same as [`execa.command()`](#execacommand-command-options) but synchronous.

Returns or throws a [`childProcessResult`](#childProcessResult).

### execa.node(file, [arguments], [options])

Run a file through a forked process.
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to #200 (comment)

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
test/fixtures/detach Outdated Show resolved Hide resolved
test/node.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

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

You also need to fix the merge conflict.

@sindresorhus sindresorhus merged commit c2f5edf into sindresorhus:master Jun 18, 2019
@sindresorhus
Copy link
Owner

Yay! Thanks for working on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .fork() method
3 participants