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

doc: refactor signal info in child_process.md #37528

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Feb 26, 2021

  • Since exec calls execFile and execFile internally calls spawn with
    options.signal, the signal parameter has been documented under exec
    as well.
  • Refactor the description of signal under all the functions.
  • Add examples of usage of signal under all the functions and add
    missing requires in the other examples.

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Feb 26, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 27, 2021

Maybe we should add the info in the YAML list as well:

  - version: v15.4.0
    pr-url: https://github.com/nodejs/node/pull/36308
    description: AbortSignal support was added.

@@ -246,6 +248,9 @@ async function lsExample() {
lsExample();
```

The `signal` option works exactly the same way it does in
Copy link
Member

Choose a reason for hiding this comment

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

An example would be nice here :]

});
controller.abort();
```
The `signal` option works exactly the same way it does in
Copy link
Member

Choose a reason for hiding this comment

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

I actually think examples in APIs are better than not - since users often don't know all the APIs they're working with - sending them to another API (spawn) when they're trying to use execFile isn't better than an example using execFile IMO.

The fact internally execFile delegates to spawn to save code internally is an implementation detail

@benjamingr
Copy link
Member

benjamingr commented Feb 27, 2021

I like adding it to missing APIs but I strongly prefer having examples in APIs to illustrate to users how to use them.

(And half the days I don't remember the difference between exec/execFile/fork/spawn myself, and I've been a collaborator for 5 years and a user for ±10 and I implemented the signal support for child_process :D)

@RaisinTen RaisinTen force-pushed the doc/add-signal-to-exec-options-in-child_process.md branch from 8a9f442 to 48ea299 Compare February 27, 2021 13:39
@RaisinTen
Copy link
Contributor Author

Maybe we should add the info in the YAML list as well:

  - version: v15.4.0
    pr-url: https://github.com/nodejs/node/pull/36308
    description: AbortSignal support was added.

@aduh95 thanks! I added it.

@RaisinTen RaisinTen force-pushed the doc/add-signal-to-exec-options-in-child_process.md branch from 48ea299 to 8b5765a Compare February 27, 2021 14:54
* Since exec calls execFile and execFile internally calls spawn with
  options.signal, the signal parameter has been documented under exec
  as well.
* Refactor the description of signal under all the functions.
* Add examples of usage of signal under all the functions and add
  missing requires in the other examples.
@RaisinTen RaisinTen force-pushed the doc/add-signal-to-exec-options-in-child_process.md branch from 8b5765a to 940959e Compare February 27, 2021 14:58
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.

LGTM with the lint issues fixed.

@RaisinTen
Copy link
Contributor Author

LGTM with the lint issues fixed.

The lint errors are fixed. Thanks for reviewing! :)

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 27, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 27, 2021

Can you add the YAML metadata for child_process.fork as well?

@RaisinTen
Copy link
Contributor Author

@aduh95
I think it has been added in #36603 already:

- version: v15.6.0
pr-url: https://github.com/nodejs/node/pull/36603
description: AbortSignal support was added.

RaisinTen added a commit that referenced this pull request Mar 4, 2021
* Since exec calls execFile and execFile internally calls spawn with
  options.signal, the signal parameter has been documented under exec
  as well.
* Refactor the description of signal under all the functions.
* Add examples of usage of signal under all the functions and add
  missing requires in the other examples.

PR-URL: #37528
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RaisinTen
Copy link
Contributor Author

Landed in 5248a17

@RaisinTen RaisinTen closed this Mar 4, 2021
@RaisinTen RaisinTen deleted the doc/add-signal-to-exec-options-in-child_process.md branch March 4, 2021 13:44
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
* Since exec calls execFile and execFile internally calls spawn with
  options.signal, the signal parameter has been documented under exec
  as well.
* Refactor the description of signal under all the functions.
* Add examples of usage of signal under all the functions and add
  missing requires in the other examples.

PR-URL: #37528
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Aug 8, 2021
* Since exec calls execFile and execFile internally calls spawn with
  options.signal, the signal parameter has been documented under exec
  as well.
* Refactor the description of signal under all the functions.
* Add examples of usage of signal under all the functions and add
  missing requires in the other examples.

PR-URL: nodejs#37528
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Sep 1, 2021
* Since exec calls execFile and execFile internally calls spawn with
  options.signal, the signal parameter has been documented under exec
  as well.
* Refactor the description of signal under all the functions.
* Add examples of usage of signal under all the functions and add
  missing requires in the other examples.

PR-URL: #37528
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants