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

child_process: add the --windows-hide flag #21314

Closed
wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

Add a flag called --windows-hide that hides console windows for newly spawned
processes by default on Windows.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @nodejs/child_process @nodejs/delivery-channels @codebytere

This should work, but I might be wrong. Thinking of adding tests for this, suggestions welcome.

Add a flag called --windows-hide that hides console windows for newly spawned
processes by default on Windows.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Jun 13, 2018
@vsemozhetbyt
Copy link
Contributor

Will this be documented?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 13, 2018

Is this in response to libuv/libuv#1878 (comment)? My idea there was that windowsHide should just default to true instead of false as it currently does.

@ryzokuken
Copy link
Contributor Author

@cjihrig it is. I'd be submitting another semver-major for that. The motivation behind this PR was that it could land as a semver-minor, allowing the electron team for get this change more quickly.

@ryzokuken
Copy link
Contributor Author

@vsemozhetbyt because this is only meant for embedders, I chose against printing out the flag in the PrintHelp function, but we should probably document it elsewhere. Would love to hear your opinions on this.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 13, 2018

I'm not really a fan of adding a new config value for this. A semver major could be released in a few months in Node 11. The patch Electron is floating to work around this is pretty trivial. I don't think it would be much of a maintenance burden to carry for a couple months.

EDIT: For reference, the patch is here, and has already been floated for 5 years.

@apapirovski
Copy link
Member

I'm in agreement with @cjihrig on this.

@ryzokuken
Copy link
Contributor Author

@cjihrig @apapirovski I'm okay with closing this if the electron peeps don't want this either and agree with you 😄

@ryzokuken
Copy link
Contributor Author

The patch Electron is floating to work around this is pretty trivial. I don't think it would be much of a maintenance burden to carry for a couple months.

@cjihrig you're right about this, and I talked to @codebytere who seemed to concur with you on this as well, so I'm closing this. Following up with a semver-major.

@ryzokuken ryzokuken closed this Jun 13, 2018
cjihrig added a commit to cjihrig/node that referenced this pull request Jun 20, 2018
This is likely the default that more Windows users are
expecting.

PR-URL: nodejs#21316
Refs: libuv/libuv#1878
Refs: nodejs#21314
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants