search

papandreou / node-pngquant

The pngquant utility as a readable/writable stream
BSD 3-Clause "New" or "Revised" License
82 stars 23 forks source link

add command options #53

Closed kmcgurty closed 4 years ago

kmcgurty commented 4 years ago

adds command options. gives ability to do { 'windowsHide': true }.

See documentation https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options

papandreou commented 4 years ago

Looks good, but I wonder if there are any other use cases than windowsHide: true? If not, maybe we should just hardcode that when spawning the child process?

kmcgurty commented 4 years ago

That link I added has some examples for what the other options do if you scroll down a bit. Maybe there's a reason someone would want to change the working directory? It doesn't complain if you don't supply anything, so I figured it can't hurt to do it this way.

kmcgurty commented 4 years ago

Also the windowsHide option is only an issue with Windows, I'm not sure if it's something you want to hard code.

For my use case, if you're running the node process through pm2, there's no console window that's open. Without that option there's a blank console window that appears on top of everything for 5-10 seconds (which is extremely annoying when you're using the computer for other things)

papandreou commented 4 years ago

Maybe there's a reason someone would want to change the working directory?

Changing the working directory won't really make a difference because this library exposes itself as a transform stream, and pngquant doesn't take any filenames except the input and output, and those are handled internally by the library. I know that was just an example, but I would rather not expose the whole options object unless there's a good reason, as it would make the library harder to understand and use correctly.

I think it would be better if it just did The Right Thing™. Now that you've pointed out that the experience on Windows is terrible without windowsHide: true, I would much rather fix that for everyone by always passing that option.

It is mapped to the UV_PROCESS_WINDOWS_HIDE flag, which according to the libuv docs is ignored on other platforms:

Hide the subprocess console window that would normally be created. This option is only meaningful on Windows systems. On Unix it is silently ignored.

kmcgurty commented 4 years ago

Okay then, we can close this PR and you can add the object in? I'm not home till tonight so it's probably better for you to do it.

papandreou commented 4 years ago

Sure, fixed in 3.1.0 :sunglasses:

Thanks for pointing out the problem. I'll port this change over to the other similar packages I maintain.