| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
This should ideally use the overload that takes a Local<Context> but OTOH, all the other options don't.
Sorry, something went wrong.
There was a problem hiding this comment.
@bnoordhuis I'll submit a follow up PR to switch all of the options to use the context.
Sorry, something went wrong.
There was a problem hiding this comment.
Likewise.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM with docs
Sorry, something went wrong.
|
It looks like the original issue is Unitech/pm2#2182 - console window pops-out when starting Node.js through PM2. Based on #7653 I added windowsHide: true to the spawn call that is used there. This PR does not fix that issue, the console window still pops up with pm2 start something.js. /cc @atrauzzi @vmarchaud |
Sorry, something went wrong.
|
@bzoz From what i remember while working on this issue (one year ago already), the "fork mode" using spawn wasn't popping up console windows. |
Sorry, something went wrong.
|
@bzoz is there any value in me keeping this open then? |
Sorry, something went wrong.
|
@vmarchaud hm, I observed the window popping out for a split-second when using pm2 start script.js. I think that uses the fork method. Now I'm confused and I don't know if I tested the right thing. @cjihrig I think we should keep this open. At least until we verify what is the exact issue here. |
Sorry, something went wrong.
|
@bzoz would you be so kind look into this issue further and assign this to yourself? It seems like you know what you have to look for and keeping a issue open to maybe look into something later on is not the best way to handle it ;-) |
Sorry, something went wrong.
|
There is discussion going on in #15217 about this |
Sorry, something went wrong.
|
I completely failed to figure out the specific conditions under which console windows pop up, much less that this flag prevents it, when I tried (see #2908 (comment)), but people do report this issue, its annoying, and would be great to have a fix for. |
Sorry, something went wrong.
The existing UV_PROCESS_WINDOWS_HIDE flag only applies to executables linked to the WINDOWS subsystem. This allows CONSOLE subsystem applications to pop up a console window. This commit sets the CREATE_NO_WINDOW process flag when UV_PROCESS_WINDOWS_HIDE to prevent this behavior. Refs: nodejs/node#15380 Refs: joyent/libuv#627 Refs: libuv#965 PR-URL: libuv#1558 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
|
This should be fixed by the next libuv update. |
Sorry, something went wrong.
|
As @bzoz writes in #15217 (comment) const assert = require('assert');
const cp = require('child_process');
{
const bat = cp.spawn('cmd.exe', ['/k', 'test.cmd'], { stdio: 'ignore', detached: true, windowsHide: true });
bat.on('exit', (code) => {
console.log(`Child exited with code ${code}`);
});
} |
Sorry, something went wrong.
|
@refack it seems like there are two problems:
|
Sorry, something went wrong.
|
Sorry, something went wrong.
|
I would prefer to prefix Windows-specific options with win32 instead of windows. |
Sorry, something went wrong.
|
Fwiw, I'm OK with just hide as the option name, or even hideWindow. |
Sorry, something went wrong.
If this will forever be a WIN32 only flag, we already have windowsVerbatimArguments as legacy, so IMHO better to keep that "convention". |
Sorry, something went wrong.
|
I agree with @refack. In fact, I picked the name windowsHide because of windowsVerbatimArguments. |
Sorry, something went wrong.
Refs: nodejs#15380 Refs: nodejs#15683 Fixes: nodejs#15394 Fixes: nodejs#15770 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Backport in #16425. The issue was that a change to the internals of spawnSync() had not been backported. |
Sorry, something went wrong.
Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
This commit exposes the UV_PROCESS_WINDOWS_HIDE flag in Node as a windowsHide option to the child_process methods. The option is only applicable to Windows, and is ignored elsewhere. Backport-PR-URL: #16425 Fixes: #15217 PR-URL: #15380 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Backport-PR-URL: #16426 Refs: #15380 PR-URL: #16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label. |
Sorry, something went wrong.
|
In light of libuv/libuv#1625, currently not well-understood, it's probably better to hold off a little longer. I'll add labels. |
Sorry, something went wrong.
|
Hm, hadn't noticed this has already been merged into v8.x. If no bugs have been reported, then I guess there is no reason to keep it out of v6.x. |
Sorry, something went wrong.
|
In the case of PM2, we added the windowsHide options to our fork an spawn calls and will soon publish it under the version 2.8.0. If there is any problem we should have people coming back to us about it, i will ping the libuv/libuv#1625 and here if that happen |
Sorry, something went wrong.
Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
| Back | FazBrowse Home | New Git URL |
This commit exposes the UV_PROCESS_WINDOWS_HIDE flag in Node as a windowsHide option to the child_process methods. The option is only applicable to Windows, and is ignored elsewhere.
This still needs docs, which I'll write if:
Fixes: #15217
Checklist
Affected core subsystem(s)
child_process