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 .fork() method #65

Closed
super-cache-money opened this issue Nov 27, 2016 · 13 comments · Fixed by #200
Closed

Add .fork() method #65

super-cache-money opened this issue Nov 27, 2016 · 13 comments · Fixed by #200
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@super-cache-money
Copy link

super-cache-money commented Nov 27, 2016

Issuehunt badges

We could just use execa('node', ['becomeTheOne.js']), but that would mean not capitalising on the added optimization that happens when using fork, as mentioned here.

There is no spoon matrix


IssueHunt Summary

[
<
i
m
g

s
r
c

'
h
t
t
p
s
:
/
/
a
v
a
t
a
r
s
3
.
g
i
t
h
u
b
u
s
e
r
c
o
n
t
e
n
t
.
c
o
m
/
u
/
2
5
4
3
5
1
1
?
v

4
'

a
l
t

'
g
m
a
r
t
i
g
n
y
'

w
i
d
t
h

2
4

h
e
i
g
h
t

2
4

g
m
a
r
t
i
g
n
y
]
(
h
t
t
p
s
:
/
/
i
s
s
u
e
h
u
n
t
.
i
o
/
u
/
g
m
a
r
t
i
g
n
y
)

h
a
s

b
e
e
n

r
e
w
a
r
d
e
d
.

Backers (Total: $60.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Owner

but that would mean not capitalising on the added optimization that happens when using fork, as mentioned here.

That is, as always with Stack Overflow answers, incorrect. .fork is just .spawn with an IPC channel. You can easily do that with .spawn too, by specifying it in the stdio option.

We should definitely add .fork() here though, as it's handy. I was thinking about it initially, but wanted to explore ways of making .fork() better and never got to it.

Pull request welcome :)

@sindresorhus sindresorhus changed the title There is no fork Add .fork() method Nov 28, 2016
SamVerschueren added a commit that referenced this issue Aug 19, 2017
@sindresorhus
Copy link
Owner

If anyone wants to take on this, see #108 for context and previous work.

@IssueHuntBot
Copy link

@IssueHunt has funded $60.00 to this issue.


@ehmicky
Copy link
Collaborator

ehmicky commented Mar 20, 2019

Just to add to the discussion, I don't think there is any "added optimization" with fork(). If you check the Node.js source code, fork() only performs some normalization logic then calls spawn(). The extra logic introduced by fork() is:

  • use process.execPath (i.e. node) as the main binary. I.e. fork('file.js') becomes spawn('node', ['file.js']). process.execPath can be modified with the execPath option.
  • use process.execArgv (i.e. node CLI arguments). I.e. if node was fired with --no-warnings, fork('file.js') becomes spawn('node', ['--no-warnings', 'file.js']). process.execArgv can be modified with the execArgv option.
  • add a silent option which is the same as using stdio: 'pipe'.
  • always use shell: false option.

So there are no optimizations. However this is convenient for two reasons:

  • it's shorter because node does not need to passed as argument
  • it re-uses the current node path (if several node binaries are installed) and CLI flags.

@sindresorhus
Copy link
Owner

@ehmicky Yes, it's mostly for convenience, but also:

The returned ChildProcess will have an additional communication channel built-in that allows messages to be passed back and forth between the parent and child.

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 28, 2019

Oh yes you're right.

This could be seen as a convenience as well since this can be achieved with spawn() by adding ipc to the stdio option, e.g. stdio: ['inherit', 'inherit', 'inherit', 'ipc'] (which is what actually fork() does under the hood).

Also to my above points, we should add that fork() defaults stdio to inherit (with the additional ipc) whereas spawn() defaults it to pipe.

GMartigny pushed a commit to GMartigny/execa that referenced this issue Apr 3, 2019
@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 10, 2019
@sindresorhus
Copy link
Owner

sindresorhus commented May 22, 2019

If anyone wants to take on this, see #108 for context and previous work.

@GMartigny @ehmicky I think you missed this. fork() is not really a good name, as fork is already a different thing in Unix: https://en.wikipedia.org/wiki/Fork_(system_call) The Node.js name is very unfortunate, since Node.js doesn't actually "fork" in the Unix sense.

@ehmicky
Copy link
Collaborator

ehmicky commented May 22, 2019

I agree that fork() is a bad name, because it does not map to Unix forks.

I think we should either:

  • try to mimic the Node.js behavior and keep the fork() name.
  • allow ourselves to deviate from the Node.js behavior and use a different name like execa.node() or execa.spawnNode().

Some things where we might consider deviating:
a) silent option defaulting to true instead of false
b) not offer any silent option at all? This option is not very useful, users can just use stdio-related options.
c) do not push ipc to stdio. It's very possible many users won't use that channel, so setting it up by default is not needed, and probably not cheap. The ones that do want it can specify it in stdio. Also half of the code in the current PR is related to this. However as @sindresorhus points it out in #108, if we don't do that, there might not be a need for this extra method at all, so we might still want to do it.

What do you think?

@sindresorhus
Copy link
Owner

allow ourselves to deviate from the Node.js behavior and use a different name like execa.node() or execa.spawnNode().

I think we should yes. Not sure which I like the most of execa.node() and execa.spawnNode() though.

b) not offer any silent option at all? This option is not very useful, users can just use stdio-related options.

I agree. We don't need it. It should just default to 'pipe' like normal.

c) do not push ipc to stdio. It's very possible many users won't use that channel, so setting it up by default is not needed, and probably not cheap.

I doubt it has any real performance impact.

@ehmicky
Copy link
Collaborator

ehmicky commented May 22, 2019

Alright, so the consequences would be:

  • rename fork() to node() or spawnNode()
  • do not add any silent option. The stdio options should default to pipe (like normal execa())

@GMartigny what do you think?

@GMartigny
Copy link
Contributor

Either .node or .spawnNode imply you can only use node as sub process. Should we get rid of the execPath option as well?

We can (and certainly should) keep execArgv to customize the sub process args.

@sindresorhus
Copy link
Owner

Should we get rid of the execPath option as well?

No, it's meant to be able to specify which Node.js executable you want. I already commented about clarifying the option description in your PR.

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Jun 18, 2019
@issuehunt-oss
Copy link

issuehunt-oss bot commented Jun 18, 2019

@sindresorhus has rewarded $54.00 to @GMartigny. See it on IssueHunt

  • 💰 Total deposit: $60.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $6.00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants