add support for killing by port (using a colon to indicate port usage) #27
Conversation
|
The build is currently failing on the Node v4 check because the package i'm using to get the pid by port is importing like this: I am the author of the package, so can release a new version that supports older nodes, but wanted to check that you definitely want to keep supporting older versions of node? |
cli.js
Outdated
| fkill(cli.input, cli.flags).catch(() => handleFkillError(cli.input)); | ||
| let pids = []; | ||
| const promises = []; | ||
| for (let i = 0; i < cli.input.length; i++) { |
There was a problem hiding this comment.
Just do for (const x of cli.input).
Either we do a major here and remove Node.js v4 support, or if we want to add this feature for v4 users too, you have to bump your module to support it. |
cli.js
Outdated
| let pids = []; | ||
| const promises = []; | ||
| for (let i = 0; i < cli.input.length; i++) { | ||
| if (cli.input[i][0] === ':') { |
There was a problem hiding this comment.
Made a little regex for this https://github.com/kevva/port-regex :).
cli.js
Outdated
| const promises = []; | ||
| for (let i = 0; i < cli.input.length; i++) { | ||
| if (cli.input[i][0] === ':') { | ||
| promises.push(portToProc(parseInt(cli.input[i].slice(1), 0))); |
There was a problem hiding this comment.
0 is an invalid radix. Use 10 for decimal numbers.
cli.js
Outdated
| promisedPids = promisedPids.filter(pid => pid); | ||
| pids = pids.concat(promisedPids); | ||
| fkill(pids, cli.flags).catch(() => handleFkillError(cli.input)); | ||
| }); |
There was a problem hiding this comment.
I would write it as:
if (cli.input.length === 0) {
init(cli.flags);
} else {
Promise.all(cli.input.map(x => x[0] === ':' ? portToProc(parseInt(x.slice(1), 10)) : x))
.then(pids => fkill(pids, cli.flags).catch(() => handleFkillError(cli.input)));
}There was a problem hiding this comment.
Overall I prefer this approach; I'll update the branch
cli.js
Outdated
| } | ||
| } | ||
| Promise.all(promises).then(promisedPids => { | ||
| promisedPids = promisedPids.filter(pid => pid); |
There was a problem hiding this comment.
What are you filtering here?
There was a problem hiding this comment.
portproc will return undefined if the input has no valid output (i.e. the port is not in use, or the pid is not using a port), so this filters out falsey responses from portproc
There was a problem hiding this comment.
Shouldn't it reject if it can't find a process?
There was a problem hiding this comment.
I'm not sure; I think if you ask to kill a valid target and an invalid target then the operation shouldn't fail; the valid target should be killed. But silently failing doesn't feel great, I agree. As a command line tool, rather than a library, perhaps it should be a little more permissive to user input? What do you think?
There was a problem hiding this comment.
I mean, your module should reject and we should handle it in here (i.e not failing), imo.
There was a problem hiding this comment.
Isn't rejecting a Promise like throwing an error? I'm not sure it's an error if you ask for the process using port X and there is no process using that port. That feels to me like it should return undefined.
If we want to treat undefined in this use case as an error, we can do - though I think i'd be more in favour or silently ignoring the command. At least if there's positive hits in the request too?
Am I making sense?
There was a problem hiding this comment.
Yeah, maybe. I usually throw but I see where you coming from. Maybe @sindresorhus @SamVerschueren has any thoughts?
Seeing as v4 is still on LTS until April next year, I have updated |
| Promise.all(cli.input.map(x => x[0] === ':' ? portToProc(parseInt(x.slice(1), 10)) : x)) | ||
| .then(pids => pids.filter(pid => pid)) | ||
| .then(pids => fkill(pids, cli.flags)) | ||
| .catch(() => handleFkillError(cli.input)); |
There was a problem hiding this comment.
This will swallow errors from portToProc which may be unintended here. We only want to handle errors from fkill.
| @@ -8,6 +8,7 @@ const psList = require('ps-list'); | |||
| const numSort = require('num-sort'); | |||
| const escExit = require('esc-exit'); | |||
| const cliTruncate = require('cli-truncate'); | |||
| const portToProc = require('portproc').portToProc; | |||
There was a problem hiding this comment.
Would have been more readable as:
const portToProcess = require('portproc').portToProcess;Not really worth saving a few characters for limited gain.
There was a problem hiding this comment.
Can you extract the CLI in portproc into a separate package? I don't want to needlessly have commander in the dependency tree. See: sindresorhus/ama#17
|
Great! Can you update the CLI help output to include support for the port stuff (readme too)? The interactive interface also needs support for ports. |
|
@coffeedoughnuts Still interested in finishing this up? :) |
|
Yep I can try to take a look this weekend
… On 22 Sep 2017, at 10:41, Sindre Sorhus ***@***.***> wrote:
@coffeedoughnuts Still interested in finishing this up? :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Closing to clean up stale PRs. Happy to reopen if you ever get to this :) |
|
Hey Just wanted to apologise for leaving this to go stale; I've been away from public code work for about a year while I moved jobs/cities/had a baby. I've refactored my package portproc to no longer include the command line utility, and also refactored the module itself to be a little more resilient; however I notice you're using a different package for port-to-proc mapping now. If there's any need to switch back to portproc, give me a shout and I can PR to put it back in (though the new package looks like it does much the same task, so will likely be fine as-is) Cheers! |
This is related to issue #19