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

Improve promise returned by the .pipe() methods #745

Closed
ehmicky opened this issue Jan 26, 2024 · 2 comments · Fixed by #834
Closed

Improve promise returned by the .pipe() methods #745

ehmicky opened this issue Jan 26, 2024 · 2 comments · Fixed by #834

Comments

@ehmicky
Copy link
Collaborator

ehmicky commented Jan 26, 2024

Problem

The following currently results in an unhandled promise rejection:

await execa('node', ['--invalidFlag', 'script.js'])
  .pipe(execa('cat'));

That's because .pipe() returns execa('cat'), so only the second child process is awaited, not the first one.

Solution

.pipe() should await both child processes instead of only the second one.

The return value can still be the second child process, which is the current behavior.

The returned promise would still resolve with the second child process' result, which is also the current behavior.
Additionally, it would now set result.pipedFrom or error.pipedFrom with the first child process' result/error.

Details

await execa('echo', ['foobar'])
  .pipe('tr', ['o', 'O'])

Would return:

{
  stdout: 'fOObar',
  exitCode: 0,
  failed: false,
  ...
  pipedFrom: {
    stdout: 'foobar',
    exitCode: 0,
    failed: false,
    ...
    // `pipedFrom` is recursive, when piping multiple processes
  },
}

When the second process fails, error.pipedFrom would contain the first process' result and output, which could be very useful for debugging.

When the first process fails, its error would be propagated as is, since this is what actually failed.


What do you think?

@sindresorhus
Copy link
Owner

👍 Looks like a good idea to me. I cannot think of any downsides.

@ehmicky ehmicky changed the title Improve promise returned by the .pipe*() methods Improve promise returned by the .pipe() methods Feb 10, 2024
@ehmicky
Copy link
Collaborator Author

ehmicky commented Feb 18, 2024

I have been working on this feature, and it appears that the .pipe() should not return the second child process. Instead, it should only resolve/reject with the second child process' result.

Taking the following example: const returnValue = source.pipe(destination), the reasons are the following:

  1. It might be confusing to users whether returnValue is source or destination, without looking up the documentation. destination makes more sense. But some users might expect source since "fluent" interfaces usually return the same value along the chain.
  2. It might be confusing to users whether returnValue methods apply to both source and destination. For example, one might expect that returnValue.kill() would terminate both source and destination.
  3. While returnValue and destination would resolve to the same value, they would need to use different promises. That's because returnValue's promise should await both source and destination. This results in an odd situation, where returnValue is the same child process as destination, and resolve to the same value as destination, but uses a different underlying promise. Not only is this confusing, but it is also tricky to implement. Since child processes cannot be cloned, the only way I could think of was to use a Proxy. This works, but comes with its own quirks.
  4. When the user passes an invalid destination argument, we would need to reject the call using a dummy child process. This is what we already do with execa() when the process fails spawning, but this is a hack. The reason we do this is so that users can expect a child process to always be returned, even when there is an error.

All of the above is solved by requiring users not to do this:

const destination = execa(...).pipe(execa(...)).pipe(execa(...))
const result = await destination

But this instead:

const source = execa(...)
const middle = execa(...)
const destination = execa(...)
const result = await source.pipe(middle).pipe(destination)

This is more explicit, clearer and avoid the problems described above.

In the majority of cases though, users only need the result, not the child process instance itself. With the new std* options, there is now almost no good reasons to use the child process directly, except from calling .kill() or using IPC. So most of the times, they will do the following, which will still work:

const result = await execa(...).pipe(execa(...)).pipe(execa(...))

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