child_process: change windowsHide default to true #21316

cjihrig · 2018-06-13T16:07:55Z

This is likely the default that more Windows users are expecting.

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

nodejs-github-bot · 2018-06-13T16:07:56Z

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/54/pipeline

cjihrig · 2018-06-13T16:23:57Z

cc: @nodejs/platform-windows @nodejs/child_process and @codebytere

refack · 2018-06-13T21:27:26Z

I'm -0 since this is a "potayto, potahto" change (RE ... likely ... users are expecting ...).
I'd rather open this as a configurable option, but we don't have a convenient way for setting protected process level globals.

refack · 2018-06-14T13:52:02Z

Since this is a Windows preference issue, I'd like to see more buy-in from @nodejs/platform-windows.

refack

Waiting for more Windows specific input.

ryzokuken · 2018-06-14T18:45:02Z

@refack I see this less as a windows issue and more of an issue for embedders. While the strange behavior is usual (and completely avoidable if you just add a single config option) for windows, this is a huge pain when it comes to consistency among platforms and behaviors for embedders.

P.S. In that spirit, /cc @nodejs/delivery-channels

refack · 2018-06-14T18:54:16Z

While the strange behavior is usual (and completely avoidable if you just add a single config option) for windows, this is a huge pain when it comes to consistency among platforms and behaviors for embedders.

I don't argue with the severity of the problem, but I want broader consensus on this specific solution.

P.S. Since this is semver-major there's no rush

BridgeAR · 2018-06-18T11:03:37Z

doc/api/child_process.md

@@ -131,6 +131,9 @@ exec('"my script.cmd" a b', (err, stdout, stderr) => {
 <!-- YAML
 added: v0.1.90
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/PR-XXXXX


These have to be changed before / while landing to https://github.com/nodejs/node/pull/21316.

cjihrig · 2018-06-18T13:41:06Z

@refack how long are you planning to leave this blocked?

This has been open for a week, blocked on feedback for 6 days, and bzoz has approved the PR.

cjihrig · 2018-06-20T14:14:45Z

Green CI: https://ci.nodejs.org/job/node-test-pull-request/15533/ 💚

BridgeAR · 2018-06-20T14:45:04Z

doc/api/cluster.md

@@ -698,6 +698,9 @@ values are `'rr'` and `'none'`.
 <!-- YAML
 added: v0.7.1
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/PR-XXXXX


This one seems like a oversight :-)

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 <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

This commit prevents console windows from being spawned when creating processes to better align with what Windows users expect and should be removed when upgrading to a version that includes nodejs/node#21316

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)

This commit prevents console windows from being spawned when creating processes to better align with what Windows users expect and should be removed when upgrading to a version that includes nodejs/node#21316

zerdos · 2018-10-31T21:29:28Z

cypress-io/cypress#2699

MarshallOfSound · 2018-10-31T23:46:40Z

Sorry y'all, missed this when it landed but worth noting now that this breaks any node script that spawns a GUI application. E.g. Electron

It makes all old releases of Electron basically un-usable on node 11 because the window never appears (see electron/electron#15467). We can explicitly set windowsHide to false in future releases but there's nothing we can do about older releases.

Just putting this here for discoverability when people start wondering why there window isn't appearing 😆

Trott · 2018-11-01T04:02:27Z

@MarshallOfSound (or anyone else on the Electron team): How big a deal is this for Electron? Is this something we should consider reverting in your opinion? Or is this a fine default and a minor nuisance that can be worked around without inconveniencing too many users?

/cc @nodejs/embedders

(Cypress is a testing tool rather than something for end users, so I'm slightly less concerned about the answer being "update to a newer version of Cypress to work with Node.js 11". But if it's a bigger problem than that attitude suggests, set me straight on that too.)

MarshallOfSound · 2018-11-01T04:40:50Z

@Trott IMO not worth reverting, this only impacts developers not production Electron apps and I think the solutions of "don't use node 11" or "upgrade to latest Electron" are perfectly reasonable when people start raising issues.

We've already landed fixes for this on Electron's side in our 1-8-x, 2-0-x, 3-0-x and 4-0-x release lines so the next release of those will fix these issues.

The question for us (Electron) to think about though is that as of Electron 5 we will be embedding node 11 so this would be a breaking change for all of our consumers as well and significantly more likely to impact Electron apps than node scripts (people spawn GUI applications all the time from within Electron apps). We might have to look at changing this default for Electron, I'll raise it at our next meeting 🤔

EDIT: I didn't actually realize this change was added kind of for us as part of an upstreaming effort, I don't think it had the intended effect (cc @codebytere )

Trott · 2018-11-01T06:24:32Z

I didn't actually realize this change was added kind of for us as part of an upstreaming effort,

Yeah, I noticed that too and got really confused....

Trott · 2018-11-02T07:21:51Z

Opened this to hopefully prompt discussion (rather than piling up comments in this merged PR): #24034

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

This commit prevents console windows from being spawned when creating processes to better align with what Windows users expect and should be removed when upgrading to a version that includes nodejs/node#21316

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

This commit prevents console windows from being spawned when creating processes to better align with what Windows users expect and should be removed when upgrading to a version that includes nodejs/node#21316

cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 13, 2018

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jun 13, 2018

TimothyGu approved these changes Jun 13, 2018

View reviewed changes

bzoz approved these changes Jun 13, 2018

View reviewed changes

apapirovski approved these changes Jun 14, 2018

View reviewed changes

trivikr approved these changes Jun 14, 2018

View reviewed changes

addaleax approved these changes Jun 14, 2018

View reviewed changes

refack previously requested changes Jun 14, 2018

View reviewed changes

ryzokuken approved these changes Jun 14, 2018

View reviewed changes

BridgeAR approved these changes Jun 18, 2018

View reviewed changes

cjihrig force-pushed the win-hide branch from 7aee394 to b01cc29 Compare June 20, 2018 13:11

BridgeAR reviewed Jun 20, 2018

View reviewed changes

cjihrig force-pushed the win-hide branch from b01cc29 to 58d2f1a Compare June 20, 2018 14:48

cjihrig force-pushed the win-hide branch from 58d2f1a to 420d8af Compare June 20, 2018 15:02

ryzokuken approved these changes Jun 20, 2018

View reviewed changes

cjihrig merged commit 420d8af into nodejs:master Jun 20, 2018

cjihrig deleted the win-hide branch June 20, 2018 15:03

This was referenced Oct 31, 2018

Cypress open dont show up window cypress-io/cypress#2667

Closed

Issue 21316 windows node11 cypress-io/cypress#2699

Merged

Trott mentioned this pull request Nov 2, 2018

Revert "child_process: change windowsHide default to true" #24034

Closed

4 tasks

lchenay mentioned this pull request Nov 12, 2018

open command has doesn't display anything in Windows 10 cypress-io/cypress#2760

Closed

ehmicky mentioned this pull request Aug 18, 2020

Windows: execa leaving processes around when running npm scripts in a sh shell sindresorhus/execa#433

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: change windowsHide default to true #21316

child_process: change windowsHide default to true #21316

cjihrig commented Jun 13, 2018

nodejs-github-bot commented Jun 13, 2018

cjihrig commented Jun 13, 2018

refack commented Jun 13, 2018

refack commented Jun 14, 2018

refack left a comment

ryzokuken commented Jun 14, 2018

refack commented Jun 14, 2018 •

edited

Loading

BridgeAR Jun 18, 2018

cjihrig commented Jun 18, 2018

cjihrig commented Jun 20, 2018

BridgeAR Jun 20, 2018

zerdos commented Oct 31, 2018

MarshallOfSound commented Oct 31, 2018

Trott commented Nov 1, 2018

MarshallOfSound commented Nov 1, 2018 •

edited

Loading

Trott commented Nov 1, 2018

Trott commented Nov 2, 2018

child_process: change windowsHide default to true #21316

child_process: change windowsHide default to true #21316

Conversation

cjihrig commented Jun 13, 2018

Checklist

nodejs-github-bot commented Jun 13, 2018

cjihrig commented Jun 13, 2018

refack commented Jun 13, 2018

refack commented Jun 14, 2018

refack left a comment

Choose a reason for hiding this comment

ryzokuken commented Jun 14, 2018

refack commented Jun 14, 2018 • edited Loading

BridgeAR Jun 18, 2018

Choose a reason for hiding this comment

cjihrig commented Jun 18, 2018

cjihrig commented Jun 20, 2018

BridgeAR Jun 20, 2018

Choose a reason for hiding this comment

zerdos commented Oct 31, 2018

MarshallOfSound commented Oct 31, 2018

Trott commented Nov 1, 2018

MarshallOfSound commented Nov 1, 2018 • edited Loading

Trott commented Nov 1, 2018

Trott commented Nov 2, 2018

refack commented Jun 14, 2018 •

edited

Loading

MarshallOfSound commented Nov 1, 2018 •

edited

Loading