-
-
Notifications
You must be signed in to change notification settings - Fork 245
[docs] add warnings about input, stdin, and stdio #1220
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
Conversation
- warn about and explain behavior when combining `input`/`fileInput` with `stdin: 'inherit'` or `stdio: 'inherit'` - warn about upcoming `stdio: [..., 'ipc']` deprecation
| This lists all options for [`execa()`](#execafile-arguments-options) and the [other methods](#methods). | ||
|
|
||
| > [!IMPORTANT] Differences from `child_process.spawn` API | ||
| > - `stdio: [..., 'ipc']` is deprecated and will produce a runtime error in version 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove both > and [!IMPORTANT]/[!NOTE]? It's not a bad thing, but it's not consistent with the rest of the documentation, which does not use this style. Thanks!
| The array can have more than 3 items, to create [additional file descriptors](output.md#additional-file-descriptors) beyond [`stdin`](#optionsstdin)/[`stdout`](#optionsstdout)/[`stderr`](#optionsstderr). | ||
|
|
||
| > [!IMPORTANT] Differences from `child_process.spawn` API | ||
| > - `stdio: [..., 'ipc']` is deprecated and will produce a runtime error in version 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Execa has quite a lot of features, which can make its documentation a little overwhelming. So we're making sure the documentation remains concise and we typically explain each piece of information only once. We also mostly avoid repeating between the api.md and rest of the documentation, and rely on users following the "More info" link instead.
Based on this, could you please write the information related to ipc in a single place, as opposed to three? The best place would be ipc.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really want to; this is the warning I would want to see as a busy developer already familiar with the child_process API encountering this library for the first time. I see burying the warning as a disservice to other experienced developers. I find it kind of annoying that y'all don't seem concerned that having subtle differences is going to cause some developers frustration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should calm down, but I guess the other thing that bothers me is, it's a real shame to abandon being a superset of the child_process.spawn API, that was a nice feature to me because that API is much simpler than execa has become and well known, so at least I can be sure what I'm getting in cases when I don't use any of execa's additional options. Maybe I should just go back to my old package that's a thinner clone of child_process plus promises...
|
|
||
| > [!NOTE] | ||
| > Be careful when combining `input` with `stdin: 'inherit'` or `stdio: 'inherit'`; | ||
| > See [this section](#combining-input-with-stdin-inherit) for details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For concision, let's assume users read this file in its entirety. If so, they will read that section below, so we do not need to point to it from those 3 sections above (and also the 2 places in api.md). This will also help keep this documentation page concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the section higher in the document then? I'm trying to look out for busy devs like me who just tend to skim for what we're looking for.
Again, because the API has been a superset of child_process.spawn, I really think devs can be forgiven for assuming they can get away without much RTFM in a case like this
|
Thanks @jedwards1211! This is very helpful. I've added a few comments to try to keep the PR small, and also consistent with the rest of the documentation. |
|
Hi again @jedwards1211, We're back from the holidays! 🎄 Thanks again for raising this issue and working on this PR. 🙏 |
|
Thanks. Honestly I think y'all should completely remove the In In This makes the behavior when both Dropping support for Letting Sindre dictate what he wants without providing any concrete rationale why left a bad taste in my mouth. 8 years of aligning with the standard library, and then one day, deciding not to for ..some.. reason |
input/fileInputwithstdin: 'inherit'orstdio: 'inherit'stdio: [..., 'ipc']deprecationfix #1218