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

Make retrieving the subprocess asynchronous #46

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Aug 29, 2024

Fixes #43.

Trying to keep the code small, although it's hard sometimes! :)

I've named the property .nodeChildProcess instead of .subprocess following your comment here: sindresorhus/execa#413 (comment). Also this would allow us to name the main return value subprocess instead of promise in the documentation, which might be easier to understand. Please let me know if this sounds good.

For example, we could document it along the following lines:

## nanoSpawn(...)

_Returns_: `Subprocess`

## subprocess

_Type_: `Promise<Result>`

### result.stdout

_Type_: `string`

## subprocess[Symbol.asyncIterator]()
## subprocess.stdout
## subprocess.stderr

_Type_: `AsyncIterable<string>`

## subprocess.nodeChildProcess

_Type_: `ChildProcess` (see Node.js doc)

index.js Show resolved Hide resolved
} catch (error) {
throw getResultError({
error,
result: initResult(),
Copy link
Collaborator Author

@ehmicky ehmicky Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the subprocess throws during spawn(), we don't have access to the .stdout and .stderr stream properties. So the only way to know whether result.stdout/result.stderr should be undefined or not is to check whether spawnOptions.stdout is either "pipe" or "overlapped".

We could do that check, but to save some bytes, this PR is making result.stdout/result.stderr an empty string instead of undefined when using things like stdout: 'ignore' or stdout: 'inherit'. Also, this would make types simpler and smaller. Finally, it could be slightly simpler for users to assume those properties are always strings.

On the other hand, there are some benefits in the "undefined vs empty string" distinction that Execa is making with result.stdout. So I'm not sure whether that's a good idea or not. 🤔 Please let me know what you think.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think empty strings are the best choice here, and would be for execa too.

test.js Show resolved Hide resolved
@ehmicky
Copy link
Collaborator Author

ehmicky commented Aug 30, 2024

Fixed merge conflict.

@sindresorhus
Copy link
Owner

I've named the property .nodeChildProcess instead of .subprocess following your comment here

I agree. That's a better choice.

@sindresorhus sindresorhus merged commit 8c076fd into main Aug 30, 2024
12 checks passed
@sindresorhus sindresorhus deleted the subprocess-async branch August 30, 2024 10:56
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 this pull request may close these issues.

Retrieve the subprocess asynchronously
2 participants