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

Re-using the features from execaNode() #781

Closed
ehmicky opened this issue Feb 3, 2024 · 5 comments · Fixed by #804
Closed

Re-using the features from execaNode() #781

ehmicky opened this issue Feb 3, 2024 · 5 comments · Fixed by #804

Comments

@ehmicky
Copy link
Collaborator

ehmicky commented Feb 3, 2024

execaNode() has some nice features:

  • prepend 'node' as the binary argument
  • re-use the current Node.js binary, including its version
  • re-use the current Node.js CLI flags, without getting any issues when using --inspect
  • forces shell: false

However, those features are not available for users who either:

  • use $ or execaCommand()
  • use synchronous methods like execaSync()
  • do not wish to add an ipc channel

We could fix this by adding a { node: true } option (any other option name works too), which would do the same thing as what execaNode() is doing, except for adding an ipc channel.

What do you think?

@ehmicky ehmicky changed the title Re-using execaNode() features Re-using the features from execaNode() Feb 3, 2024
@sindresorhus
Copy link
Owner

Why would you not want a ipc channel though? If there's no good reason, then having {node: true} be the same as execaNode() would make the docs simpler.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Feb 3, 2024

The only reason I was thinking of was to avoid the cost of setting up the IPC channel.

A named pipe is created (or is a TCP Unix socket used? I am not sure from looking at the libuv code). I am not sure whether this counts towards ulimit -n. 🤔

Performance-wise, the child process does some additional setup when it starts. I tried benchmarking it with the following code.

import {spawn} from 'node:child_process'
import {once} from 'node:events'

console.time()
for (let i = 0; i < 1e3; i += 1) {
  const childProcess = spawn('node', ['empty.js'], {stdio: ['pipe', 'pipe', 'pipe', 'ipc']})
  await once(childProcess, 'exit')
}
console.timeEnd()

console.time()
for (let i = 0; i < 1e3; i += 1) {
  const childProcess = spawn('node', ['empty.js'], {stdio: ['pipe', 'pipe', 'pipe']})
  await once(childProcess, 'exit')
}
console.timeEnd()

Which gives me (Node 21.1.0, Ubuntu):

default: 18.379s
default: 15.602s

I am not sure though whether the reasons above are enough to justify creating an ipc channel by default or not. 🤔 How do you feel about this?

@sindresorhus
Copy link
Owner

People can just do {node: true, ipc: false} if they don't want it, right? If so, I think the consistency is worth the tiny performance overhead by default.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Feb 3, 2024

Yes, that's a good point. Also, the benchmark is on an empty file. The time to actually load the file should be much higher, making the time spent setting up the ipc channel not as significant.

So, what are you suggesting is the following, is this correct?

  • ipc option defaults to true if {node: true} is used
  • execaNode(...) is the same as execa(..., {node: true})

@sindresorhus
Copy link
Owner

Yes

And make sure there is a test for {node: true, ipc: false}.

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 a pull request may close this issue.

2 participants