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

Allow stdin/stdout/stderr options to be a file URL #592

Closed
ehmicky opened this issue Dec 14, 2023 · 3 comments · Fixed by #621
Closed

Allow stdin/stdout/stderr options to be a file URL #592

ehmicky opened this issue Dec 14, 2023 · 3 comments · Fixed by #621

Comments

@ehmicky
Copy link
Collaborator

ehmicky commented Dec 14, 2023

New feature

Allow the stdin, stdout and stderr options to be new URL('file://path'). It would be transformed to a stream using fs.createReadStream()/fs.createWriteStream().

Removed features

The above would replace the inputFile option.
Combined with #591, this would also allow removing the .pipeStdout(), .pipeStderr() and .pipeAll() methods.

Rational

Why remove the current .pipe*() methods and replace them with #591 and this? Those methods have multiple problems.

Unhandled errors

When piping two processes, any error thrown on the first process is unhandled, making the process crash.

try {
  await execa('crash!').pipeStdout(execa('cat'))
} catch (error) {
  // This is not handled
}

This is because .pipeStdout() returns the second child process. Any error thrown on the child process is left unhandled.

Ignored stderr

For the same reason, any stderr from the first child process will be ignored, which might or might not be intended. In many cases, this could hide some bugs.

From an efficiency standpoint, this means the first child process's stderr uses the default pipe option, even though it behaves as if ignore had been used, except that a stream is unnecessarily created.

Race condition

When piping to a stream (including the one created when passing a file path), the Execa promise resolves once all data has been sent to the stream, but not when that stream has finished processing that data.

For example, when await execa(...).pipeStdout('filePath') resolves, the file might not exist yet. This is currently caught by our tests for .pipeStdout(filePath) which randomly fail.

Complexity

Users currently have too many ways to pipe. While each of them serves different purposes, the API is not orthogonal and can be confusing. We offer: childProcess.pipeStdout(), childProcess.pipeStderr(), childProcess.pipeAll(), childProcess.stdout|stderr|all.pipe(), the all option, the input option, the inputFile option and the stdin/stdout/stderr options.

This proposal would simplify it to:

  • For most use cases: the stdin/stdout/stderr options are enough
  • For advanced use cases: use childProcess.stdout|stderr|all.pipe()
  • To pipe multiple processes: use Stream.pipeline(childProcess...)

Piping stdout and stderr separately

Piping stdout and stderr separately might be confusing. For example, the following works:

execa(...).pipeStdout('stdout.txt').pipeStderr('stderr.txt')

But the following (potentially unexpectedly) redirects the second (not the first) process's stderr to the third process's:

execa(...).pipeStdout(execa(...)).pipeStderr(execa(...))

Piping to stream's confusion

When piping to another child process, that other child process is returned. When piping to a stream, that stream is not returned. Making both behave the same would be less confusing, and give users more the impression that child process behave like streams when using Execa.

@sindresorhus
Copy link
Owner

👍

@ehmicky
Copy link
Collaborator Author

ehmicky commented Dec 16, 2023

Started with stdin at #610 and #614.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Dec 18, 2023

stdout/stderr done at #621.

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