-
Notifications
You must be signed in to change notification settings - Fork 31k
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
child_process: add subprocess.readLines
method
#45774
base: main
Are you sure you want to change the base?
Conversation
I'm very much surprised to see that #42590 was merged, despite the very valid discussion there ... can we not make the same mistake again? If a utility method is useful, then I see no reason not to make it composable; e.g., a |
You can already do that with |
For errors from spawning, this seems reasonable, for the other causes of
Again… would you want this? Wouldn’t you be more likely to want to check the exit code yourself after the stream has finished out? It seems like a bit of a footgun that the async iterable can fail at the end of the stream.
To be clear, by “mistake” I was referring to merging a PR with outstanding discussion, even if not in the form of “Requested Changes”, in the way #42590 was merged. |
lib/internal/child_process.js
Outdated
this.on('error', () => {}); | ||
} else { | ||
const errorPromise = new Promise((_, reject) => this.once('error', reject)); | ||
const exitPromise = new Promise((resolve, reject) => this.once('exit', (code) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need to listen for close
, not exit
here, otherwise you’d run the risk of dropping data.
I guess in this case, folks should use
Ah yes, sorry for the misunderstanding, in that case of course I agree. |
48097dc
to
038bd4c
Compare
|
Similarly to
fileHandle.readLines()
, iterating line by line over the output of a command is a somewhat regular thing when writing scripts. This PR adds a util method to cover this use case.