search

jbenet / node-subcomandante

taaaaake ooooooon meeee
4 stars 8 forks source link

Listen to `end` event over `finish` for child process stdio #6

Closed travisperson closed 6 years ago

travisperson commented 6 years ago

The finish event is documented only for stream.Writable, and is not associated with stream.Readable per the stream documentation.

The current behavior appears to be undocumented and recently changed with in nodejs v9.9.0[0].

[0] https://github.com/nodejs/node/commit/9c0c0e68ac

I opened an issue against nodejs/node, if the issue is closed wontfix, or similar this PR will need to merge to allow for usage on nodejs versions > v9.9.0. This currently affects the ipfs/js-ipfs testing on later versions of nodejs.

https://github.com/nodejs/node/issues/19764

lpinca commented 6 years ago

I think this should be merged regardless of what happens with https://github.com/nodejs/node/pull/19772 because:

  1. subprocess.stderr and subprocess.stdout are technically read only streams.
  2. This is needed for Node.js 10+
travisperson commented 6 years ago

Ya, we'll get this fixed on our end. If you don't want to land https://github.com/nodejs/node/pull/19772, we won't have any issue.

lpinca commented 6 years ago

I would prefer not to land nodejs/node#19772 but it depends on how much disruption the change created, and on other Node.js collaborators.

dignifiedquire commented 6 years ago

I would suggest using https://www.npmjs.com/package/end-of-stream in this package to make this more robust

daviddias commented 6 years ago

Meanwhile. @jbenet we need merge and publish perms for this module

dignifiedquire commented 6 years ago

Meanwhile. @jbenet we need merge and publish perms for this module

Probably be good to move this to the shipyard