ehmicky/nvexeca

Add synchronous execution support

nicolas-goudry opened this issue · 11 comments

Which problem is this feature request solving?

I need to wait for a command to close before continuing executing my code.

I made an easy and clear test to show what is going on:

parent.mjs

import nvexeca from 'nvexeca'

console.log('parent: going to wake up child...')

await nvexeca('6', 'node', ['./child.js'], { stdout: 'inherit' })

console.log('parent: child is up!')

child.js

console.log('child: ZzZzZzZz')

setTimeout(() => {
    console.log('child: I’m up!')
}, 5000)

Actual output:

parent: going to wake up child...
parent: child is up!
child: ZzZzZzZz
child: I’m up!

Expected output with synchronous execution:

parent: going to wake up child...
child: ZzZzZzZz
child: I’m up!
parent: child is up!

Describe the solution you'd like

Maybe expose a sync function that calls execa’s sync method?

Actually, this is working as expected with execa:

await execa.sync('node', ['./child.js'], { stdout: 'inherit' })

Describe alternatives you've considered

I thought about watching over childProcess.exitCode to be updated from null to subprocess exit code. But that seems tricky and hack-ish... I think it would involve some Observable maybe? I did not dig into this solution/workaround yet.

Can you submit a pull request?

Maybe I can try to dig into sources to add this feature over next weekend, but can’t make any promises!

Well, in fact, I took a quick look at the code and it didn’t seem too long to implement. So, I did it, but now I’m stumbling on writing a test for the feature… Could you please help me?

I did it on a new branch of my fork: https://github.com/nicolas-goudry/nvexeca/tree/feat/sync-option

What I’m trying to do is to use a helper file as test script, which just waits for 2 seconds and does nothing else.

const TIMEOUT = 2000

;(function sync() {
  setTimeout(() => {}), TIMEOUT)
})()

export {}

But the linter is complaining about the empty arrow function. I tried using a console.log('') but it doesn’t work either.

I could really use some help on this! Thanks

Hi @nicolas-goudry,

Thanks for filing this issue.

This would require not only calling execa.sync() instead of execa(), but also making the following two methods synchronous. This might be quite a lot of additional code. Also, using sync I/O and top-level logic is generally not a best practice.

Taking your initial example, is following the promise possible?

import nvexeca from 'nvexeca'

console.log('parent: going to wake up child...')

nvexeca('6', 'node', ['./child.js'], { stdout: 'inherit' }).then(() => {
  console.log('parent: child is up!')
})

Hi @ehmicky,

Thanks for replying so promptly!

I don’t see why those functions would need to be synchronous as they are already awaited?
I think there is a misunderstanding by what I call « synchronous » or a misuse of it, as this option would still return a promise IMO.

For reference, I’d use it this way:

import nvexeca from 'nvexeca'

console.log('parent: going to wake up child...')

await nvexeca('6', 'node', ['./child.js'], { stdout: 'inherit', sync: true })

Regarding your proposition, as the result is the same between following the promise or awaiting it, it would not match my use case.

papb commented

This looks more like a bug report than a feature request to me. Since you called await, it should work. I think @ehmicky misread your issue. Although we usually use the word synchronous for things that do not use async-await. In this case it seems you just want await to really wait until the subprocess is done, right?

Yes, I did misread your issue, thanks for clarifying @nicolas-goudry and @papb 🙏

Would the following work for your case?

import nvexeca from 'nvexeca'

console.log('parent: going to wake up child...')

const { childProcess } = await nvexeca('6', 'node', ['./child.js'], { stdout: 'inherit' })
await childProcess

console.log('parent: child is up!')

See also https://github.com/ehmicky/nvexeca#example

@papb Yes, I want to wait for the subprocess to be done 👌

@ehmicky This is exactly what I needed! I don’t know how I missed it while reading the docs though… Seems a little weird to await twice, but given that nvexeca returns the original promise from execa into the childProcess property, that makes sense.

Thanks for your help and sorry for the inconvenience!

The reason is that nvexeca does two things:

  1. Download the Node.js binary locally
  2. Call execa() with it

At the same time, users might need to retrieve the childProcess without awaiting it if they want to manually stream stdout/stderr (see examples). If we were to await childProcess, it would not be possible for them to do so.

I have created a PR to improve the README.md. Would you like to review it?

@all-contributors Please add @nicolas-goudry for helping improving the doc

@all-contributors Please also add @papb for helping answering the question

@ehmicky

I've put up a pull request to add @papb! 🎉