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

Incorrect documentation for child_process #22297

Closed
thw0rted opened this issue Aug 13, 2018 · 2 comments
Closed

Incorrect documentation for child_process #22297

thw0rted opened this issue Aug 13, 2018 · 2 comments
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. question Issues that look for answers.

Comments

@thw0rted
Copy link
Contributor

thw0rted commented Aug 13, 2018

  • Version: all
  • Platform: all
  • Subsystem: child_process

I recently put in a PR over on DefinitelyTyped based on the child_process docs, and another user pointed out that the documented options are missing a legal value. The stdio section of the child_process docs lists "inherit" as a valid (shorthand) string argument, but it does not appear in the numbered list under the array argument description.

At first, I thought "inherit" and null/undefined were equivalent, but then I dug around in the relevant code and it behaves differently from any other value. Under the hood, the transformed object (having type = "inherit") is passed through to the native implementation as is, and I'm not sure where to find the source for that (in v8, I guess?), so the trail sort of went cold for me. If anybody can explain what "inherit" (in an array value, not as shorthand) actually does, I can put in the PR, or you could just file the PR directly.

@vsemozhetbyt vsemozhetbyt added question Issues that look for answers. child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Aug 13, 2018
@vsemozhetbyt
Copy link
Contributor

cc @nodejs/child_process

@gireeshpunathil
Copy link
Member

@thw0rted - from JS land, it reaches to uv_spawn of libuv through this route, with the options encoded in uv_process_options_t

  * frame #0: 0x00000001009577fa node`uv_spawn
    frame #1: 0x00000001000ae5a7 node`node::(anonymous namespace)::ProcessWrap::Spawn(v8::FunctionCallbackInfo<v8::Value> const&) + 2445
    frame #2: 0x000000010022822f node`v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) + 623
    frame #3: 0x0000000100227771 node`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 689
    frame #4: 0x0000000100226dc0 node`v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) + 288

even without digging into that method, you could actually fill the missing one in the numbered list by searching for inherit in the same document file as it its behavior is documented elsewhere.

Hope this helps. Expecting a doc PR from you soon!

thw0rted added a commit to thw0rted/node that referenced this issue Aug 14, 2018
rvagg pushed a commit that referenced this issue Aug 15, 2018
PR-URL: #22309
Fixes: #22297
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

3 participants