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

Added getExistingProcesses() #454

Closed
wants to merge 1 commit into from
Closed

Added getExistingProcesses() #454

wants to merge 1 commit into from

Conversation

millerm
Copy link

@millerm millerm commented Aug 16, 2016

First attempt at a contribution, addressing issues/441. Added a function to run the unix commands provided by @vjeux. It displays the names of existing processes running on default port in the start script. Tested manually. Eager for critiques and ideas on how to clean it up.

Edit: only made addition for osx

@ghost
Copy link

ghost commented Aug 16, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost ghost added the CLA Signed label Aug 16, 2016
@mxstbr
Copy link
Contributor

mxstbr commented Aug 16, 2016

Main concern: Does this work on Windows/Linux?

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sotojuan
Copy link

sotojuan commented Aug 16, 2016

It's another dependency, but execa has better Windows support than child_process.

function getExistingProcesses(port) {
//determine if another process running on the desired port
return new Promise(function (resolve, reject) {
var command = 'lsof -P | grep TCP | grep :' + port + ' | cut -d \' \' -f 1';
Copy link

@TryingToImprove TryingToImprove Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lsof, grep or cut is not valid windows commands..

netstat might be helpful, by requires administrative privileges..

C:\WINDOWS\system32>netstat -aon | findstr 3000
  TCP    127.0.0.1:3000         0.0.0.0:0              LISTENING       15564

C:\WINDOWS\system32>tasklist /fi "pid eq 15564"

Image Name                     PID Session Name        Session#    Mem Usage
========================= ======== ================ =========== ============
node.exe                     15564 Console                    5    125.580 K

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also run the second line I mentioned this way you get the entire command that started it instead of just "node". This way we get the entire path of the app running which should be more helpful :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For windows support, let's make it work on one platform and have the infra in place so that someone using windows can chime in and make it work over there with a good user experience!

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Hi @millerm, would you be interested in addressing the above comments?

@ghost ghost added the CLA Signed label Sep 2, 2016
@ghost ghost added the CLA Signed label Sep 2, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

I’m going to close since this has gotten out of date, and review comments weren’t addressed.
Feel free to resubmit this with fixes if you find the time!

@gaearon gaearon closed this Sep 30, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants