Spawn detached window hide option #2908
Conversation
sam-github
added
child_process
| @@ -219,6 +219,7 @@ namespace node { | |||
| V(verify_error_string, "verifyError") \ | |||
| V(version_string, "version") \ | |||
| V(weight_string, "weight") \ | |||
| V(windows_hide_string, "hide") \ | |||
There was a problem hiding this comment.
Should this just be hide_string?
There was a problem hiding this comment.
I was modelling it after the UV constant name, but I see that hide_string would be better, done.
|
LGTM. |
|
I could assert that if If there is no detached window, its a harmless nul-op. I'm not sure if asserting would make the API more predictable and avoid people wondering why But you think it would help people if they were hoping that |
Yeah, I don't really see the point of that. |
|
Virtual shrug, no strong opinion. Any future bug reports, I'll assign them to you guys. |
|
I'm +1 on this, but since I suggested it in the first place I'd like to get another LGTM from @orangemocha or @joaocgreis or @mscdex . |
|
sort of related #556 |
| @@ -366,8 +366,11 @@ callback or returning an EventEmitter). | |||
| * `env` {Object} Environment key-value pairs | |||
| * `stdio` {Array|String} Child's stdio configuration. (See | |||
| [below](#child_process_options_stdio)) | |||
| * `detached` {Boolean} The child will be a process group leader. (See | |||
| * `detached` {Boolean} Prepare child to run independently of it's parent | |||
|
One minor nit, but otherwise LGTM. |
sam-github
force-pushed
the
spawn-detached-window-hide-option
branch
from
ea16d4a
to
dc4e069
Compare
|
Generally LGTM. Regarding the semantics of hide when not detached, I have two suggestions, alternative to each other:
|
|
2 seems somewhat easier to use, and more expressive of the intent, but slightly disconnected from uv. Also, neither detached or background really do anything like what the option name would suggest on non-Windows. I kindof like that about hide, its clearly a windows specific variant of attached (edit: I can write code that sets detached and hide, and it will work on v0.10, there will just be an ugly window, and will work better once this PR merges. For background, I would have to set detached and background, so its all about the same to me (as long as the combination is supported - if I have to start do node version checking to see what options to pass I'll be annoyed!). I think I just talked myself into prefering @orangemocha's option 1. :-) |
Just to clarify, |
|
@orangemocha I understand, but I would always pass both, because then it would work on v0.10 and v4.wherever-this-is-released. |
|
@sam-github : got it now. It makes sense -considered the need for backward compatbility- that an additive property like |
Reassigning a named parameter while also using the arguments object causes the entire function to never be optimized. PR-URL: nodejs#4613 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
To enable greater parallelization of tests on CI, move resource intensive tests out of parallel and into sequential. Ref: nodejs#4476 PR-URL: nodejs#4615 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: nodejs#4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#4617 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Brian White <[email protected]>
Do not attempt to read data from the socket whilst on OpenSSL's stack, weird things may happen, and this is most likely going to result in some kind of error. PR-URL: nodejs#4624 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>
Some variables are declared with var more than once in the same scope. This change reduces the declarations to one per scope. PR-URL: nodejs#4633 Reviewed-By: jasnell - James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
A 50ms timeout results in a race condition. Instead, enforce expected order through callbacks. This has the side effect of speeding up the test in most situations. Ref: nodejs#4476 PR-URL: nodejs#4637 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#4639 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chris Dickinson <[email protected]>
This comment was added with an assumption that we could determine the IP address that localhost should resolve to without performing a lookup. This was a false assumption and should be removed. PR-URL: nodejs#4648 Reviewed-By: Brian White <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Prior to this commit, the test was flaky because it was executing the majority of its logic in a function called from the client and multiple events on the server. This commit simplifies the test by separating the server's connection and listening events, and isolating the client logic. Refs: nodejs#4476 Refs: nodejs#4644 PR-URL: nodejs#4650 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Detached children are run in a new console window by default. For
children that are intended to run in the background with no console I/O,
this window is an annoying pop-up, so uv allows it to be hidden.
I can't think of a way to test this other than manually, which I still have to do, but @piscisaureus, this is what you suggested (I think).
/cc @nodejs/platform-windows
Includes a bit of the docs from #2903.