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.

Sign up for GitHub

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

Jump to bottom

Feat/exec stdio #48553

Closed
wants to merge 3 commits into from
Closed

Feat/exec stdio #48553

wants to merge 3 commits into from

Conversation

Ceres6
Copy link
Contributor

@Ceres6 Ceres6 commented Jun 26, 2023 •
edited
Loading

This PR attempts to add input and stdio options to child process async method who hadn't while its sync counterparts did. spawn only input and exec and execFile both.

Fixes #45306
Fixes #25231

@Ceres6
child_process: add stdio to exec and execFile options
8ff28e7 
exec and execFile methods changes to accept stdio in its options as its
corresponding sync methods. Tests added and docs updated
@Ceres6
child_process: input added to async functions
6a88815 
input option added to spawn, exec and execFile. tests and documentation
updated

Fixes: nodejs#45306
Fixes: nodejs#25231
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Jun 26, 2023
@Ceres6 Ceres6 marked this pull request as draft June 26, 2023 00:46
@Ceres6 Ceres6 marked this pull request as ready for review June 27, 2023 08:16
@bnoordhuis
Copy link
Member

There was a lot of disagreement in #25231 so... 🤷

@Ceres6
Copy link
Contributor Author

Ceres6 commented Jul 3, 2023

@bnoordhuis most of the disagreement was around how this was going to be implemented. I think this could be a good opportunity to discuss the matter on an actual implementation option. I think this could help migrating sync code to async and that there aren't any downsides to having an extra option.

@mscdex
Copy link
Contributor

mscdex commented Jul 3, 2023

I think this could help migrating sync code to async

I think people who are using the synchronous methods are using them for the synchronous behavior, not because of conveniences missing from the asynchronous versions. That means it's not generally going to be a simple find/replace to convert that kind of code.

@mscdex mscdex mentioned this pull request Jul 15, 2023
Closed
@ehmicky
Copy link

ehmicky commented Jul 15, 2023 •
edited
Loading

Related: #48786
Also, the current implementation might encounter the problem highlighted in #40085

benjamingr
benjamingr requested changes Jul 18, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Sorry, but Ben explained pretty well in #48786 (comment) why this isn't a great idea most likely.

vidddd1
vidddd1 reviewed Sep 3, 2023
View reviewed changes
Copy link

@vidddd1 vidddd1 left a comment

Choose a reason for hiding this comment

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

@Ceres6 Ceres6 closed this Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vidddd1 vidddd1 vidddd1 left review comments

@mscdex mscdex mscdex left review comments

@benjamingr benjamingr benjamingr requested changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
7 participants
@Ceres6 @bnoordhuis @mscdex @ehmicky @benjamingr @vidddd1 @nodejs-github-bot