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 - fixes #65 #108

Closed
wants to merge 2 commits into from
Closed

Add fork method - fixes #65 #108

wants to merge 2 commits into from

Conversation

SamVerschueren
Copy link
Contributor

@SamVerschueren SamVerschueren commented Aug 19, 2017

It's a start for the execa.fork method. Still need some extra work though but would like some feedback at the initial approach.

One thing I noticed with the stdio option of the original child_process.fork method is that it looks like it ignores the option whenever it's a string. So passing ignore to stdio results in [0, 1, 2, 'ipc']. Even passing stdio: 'foo' results in the same behaviour.

Fixes #65

@SamVerschueren
Copy link
Contributor Author

I expanded the functionality in the latest commit. Something I missed in the first place is that the first argument of .fork() is the modulePath to a JavaScript file while the first argument of spawn is the command. So currently, I implemented it the exact same way. Not sure if we want to improve this?

@SamVerschueren
Copy link
Contributor Author

I was thinking this through once again. Actually I don't like an extra method, it just confuses people. If you google for it, a lot of SO posts can be found regarding fork Vs. spawn Vs. execFile. So maybe it would be better if we just add an extra option to the default execa method, something like ipc: true which would always set the 4th argument of the stdio array to ipc. Just a suggestion, but I think it's worth a discussions.

// @kevva @jfmengels @jamestalmage

@sindresorhus sindresorhus reopened this Oct 5, 2017
@sindresorhus
Copy link
Owner

@SamVerschueren It's not just the IPC though, it also spawns the Node.js binary only, so it's a bit different from execa(). How about execa.node()? Is that clear enough?

@SamVerschueren
Copy link
Contributor Author

Totally missed this one :).

How about execa.node()? Is that clear enough?

It's clear that it only spawns the node binary, but it's not clear that it opens an IPC channel. What we could do however, is adding an option to execa.node() like I suggested in my previous post, ipc: true to make it explicit. So using execa.node() would just work the same way as execa('node'). Setting ipc: true would add the IPC channel.

@sindresorhus
Copy link
Owner

Even if it's not clear from just seeing the method, we could make it clear in the docs about the behavior. I don't see the point of a .node() method if I have to set ipc: true manually. Then execa('node') is just as short.

@sindresorhus
Copy link
Owner

Closing this to clean up open stale PRs.

@sindresorhus sindresorhus mentioned this pull request Aug 27, 2018
@sindresorhus sindresorhus deleted the iss65 branch June 23, 2019 13:05
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.

2 participants