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 readLines(stream: Readable): AsyncIterable<string>. This could be used as for await (const line of readLines(subprocess.stdout)) { ... }, but, and this is the important bit, also with any other stream.

This could be used as for await (const line of readLines(subprocess.stdout)) { ... }

You can already do that with import { createInterface as readLines } from 'node:readline'. However, with that you are still lacking support for catching an 'error' event on the ChildProcess instance, and the exit code not being 0.
My own opinion is that fileHandle.readLines is a great addition to the Node.js fs API, so I would very much be in favor of making the same mistake again, especially because I can't see a single downside to such mistake. That being said, if we can find a a better API for that, I'm open to suggestions.

However, with that you are still lacking support for catching an 'error' event on the ChildProcess instance,

For errors from spawning, this seems reasonable, for the other causes of ChildProcess errors (kill()ing the child process fails/sending messages to it fails), I would be surprised if a user expected that type of error to cause the async iterable to fail.

and the exit code not being 0.

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.

making the same mistake again

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.

For errors from spawning, this seems reasonable, for the other causes of ChildProcess errors (kill()ing the child process fails/sending messages to it fails), I would be surprised if a user expected that type of error to cause the async iterable to fail.
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.

I guess in this case, folks should use readLines(cp.stdout) instead of cp.readLines(). The fact that it doesn't cover all the use cases doesn't strike me as a sign we should add this method. Worth noting that execa does consider a non-zero exit code as a failure or if it's killed by a signal, and given the popularity of this package (65M/week on npm), I think it's fair to assume it's a quite common use case and makes sense as a default.
This feature is designed to improve scripts authoring DX (same as fileHandle.readLines()), it may not make sense in a production context, but I think that's OK, people can still use the old APIs and/or use the options to dictate the API to behave appropriately.

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.

Ah yes, sorry for the misunderstanding, in that case of course I agree.

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.