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

add logging of existing default port process on start #816

Merged
merged 15 commits into from
Nov 22, 2016

Conversation

ianmcnally
Copy link
Contributor

@ianmcnally ianmcnally commented Oct 1, 2016

Hello awesome contributors,

In #441, there are was a request to guess the name of the existing process running on the default port when starting CRA. So, building on the incomplete yet solid work done in #454, this PR aims to complete that feature.

with a CRA app process found

screen shot 2016-11-02 at 16 37 20

It shows the app's name and directory.

Note: my-test-app is the name of a CRA app in ~/dev/test2

with a non-CRA process found

screen shot 2016-11-02 at 16 39 36

It shows the command name and directory.

without a process found

screen shot 2016-10-01 at 14 47 15

It shows the original, generic message.

@facebook-github-bot
Copy link

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 [email protected]. Thanks!

@ianmcnally ianmcnally changed the title add logging of existing port process on start add logging of existing default port process on start Oct 1, 2016
@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!

@gaearon
Copy link
Contributor

gaearon commented Oct 1, 2016

Let's keep the message intact but add a colon at the end and print the process name indented with two spaces on next line?

This way the line wouldn't be so long.

What do you think?

@ianmcnally
Copy link
Contributor Author

Yeah, I think that's better. The original wording, taken from #454, felt a little off with the whole line. Changing now. Thanks!

@ianmcnally
Copy link
Contributor Author

@gaearon in dc824f5, it now reads:

screen shot 2016-10-02 at 17 05 24

thanks and let me know if that's what you were looking for.

@gaearon
Copy link
Contributor

gaearon commented Oct 3, 2016

I would try to make second line of different color (cyan?).

Also, if the command matches our own (so we know it's a CRA repo), is there any way to figure out which directory it is running from? It would be nice to just display the repo name/directory instead if we know for sure this is another CRA instance.

Ian McNally added 2 commits October 4, 2016 10:18
- With the currently process filtering, if multiple processes are returned as running on port 3000, this command would fail. This splits apart the process IDing and the process naming, to support multiple processes.
- One curious thing about the bash command to get processes, is that it'll include browsers with a window open on localhost:3000. May want to reconsider that.
@ianmcnally
Copy link
Contributor Author

ianmcnally commented Oct 4, 2016

Hey Dan,

In abeca8a I add coloring to the process name, as requested:

screen shot 2016-10-04 at 10 20 03

That uncovered a couple things, mentioned in the last commit:

  • As it was, if multiple processes are returned as running on port 3000, this command was failing. I split apart the process IDing and the process naming, to support multiple processes.
  • One curious thing about the bash command is that it'll include browsers with a window open on localhost:3000. May want to reconsider the command, though I'm not an expert on that.

So, as far as wrapping this ticket up, I'm wondering what you consider the MVP. Could we live with this as is, and refine both the repo/directory identification and the browser-caught-as-a-process? Or would you like to see one or both of them solved before merging this in?

@ianmcnally
Copy link
Contributor Author

Wanted to leave a note here. I did get something working for getting the working directory of a process. Maybe we can solicit feedback on it?

With CRA already running:

$ P=`lsof -i:3000 -P -t`; ps -o command -p $P | sed -n 2p | tr -d "\n"; lsof -p $P | grep cwd | awk '{print " in " $9}'

node packages/react-scripts/scripts/start.js in /Users/ian/dev/create-react-app

Based on my Googling, lsof with grep is the only mac-compatible way to search for the directory.

@ianmcnally
Copy link
Contributor Author

ianmcnally commented Oct 6, 2016

hey @gaearon, based on my notes above, I added the command's directory in the output in 504efb6:

screen shot 2016-10-06 at 11 05 35

I thought coloring them differently would help readability, but I guessed on colors. Let me know what you think.

The only remaining quirk is that this'll show your browser if you're on localhost:3000. Wondering if that's an acceptable tradeoff and we can :shipit: ?

- also moved terminal coloring up, when getting the process, to be able to distinguish the process command from the directory
@ianmcnally
Copy link
Contributor Author

Pushed a slight tweak in e3dcaaf to only color "in" since it's our wording. And keeping the process command and directory in the same color visually links them.

E.g.,

screen shot 2016-10-06 at 18 24 07

- better reflects its broadened scope (both command and directory)
@ianmcnally
Copy link
Contributor Author

In 4cfe5c3, I went one step further and customized the process output when it's create-react-app:

screen shot 2016-10-09 at 18 12 21

@ianmcnally ianmcnally force-pushed the guess-existing-port-process-441 branch from aa899b6 to 9c2e3aa Compare October 10, 2016 15:17
@ianmcnally
Copy link
Contributor Author

ianmcnally commented Oct 10, 2016

backed off the custom CRA message (above) since a CRA-based app doesn't have the same access to CRA's package.json.

@gaearon
Copy link
Contributor

gaearon commented Oct 11, 2016

backed off the custom CRA message (above) since a CRA-based app doesn't have the same access to CRA's package.json.

I don't see a commit that backs it off, did you push it?

Can you clarify in more detail why it didn't work?

@gaearon gaearon added this to the 0.8.0 milestone Nov 20, 2016
@fson
Copy link
Contributor

fson commented Nov 22, 2016

I tested starting the dev server for a CRA project and then trying to start another one, got this message:
screen shot 2016-11-22 at 12 26 11

I don't know why Chrome is there. Perhaps because the web app was open in Chrome? Can we distinguish between listening on a port and being connected to it?

I ran lsof -i :3000 -P with react-scripts and Chrome running and it looked like this:
screen shot 2016-11-22 at 12 39 48
Maybe we can look for the string LISTEN to narrow down the list of processes?

@fson
Copy link
Contributor

fson commented Nov 22, 2016

Looks like we can pass the -sTCP:LISTEN option to lsof to only get processes listening on the port:
screen shot 2016-11-22 at 12 46 08

}

function getProcessIdsOnPort(port) {
return execSync('lsof -i:' + port + ' -P -t', execOptions).match(/(\S+)/g);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's modify the command so that we only get listening processes. Adding -sTCP:LISTEN should do the trick.

Otherwise looks great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

excellent find, thanks @fson. i'll add it in

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to detect/support Windows here?

Copy link
Contributor

@gaearon gaearon Nov 22, 2016

Choose a reason for hiding this comment

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

It's fine to not support Windows for "niceties" like this as long as there's a reasonable fallback and it doesn't crash.

Somebody can then add Windows-specific code in a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, just want to make sure it doesn't crash.

- Removed the handling of multiple process IDs since you can filtering by listening process (and not have the browser in the list of processes)
- Trimmed the terminal outputs for better matching (process id) and better terminal output (directory of process)
@ianmcnally
Copy link
Contributor Author

ianmcnally commented Nov 22, 2016

Incorporated the feedback from @fson on "listen" filtering in 89ffa37, and took the opportunity to clean up the handling of multiple matching IDs.

Now, with a CRA app and a browser open on localhost:3000, it looks like:

screen shot 2016-11-22 at 11 53 38

which looks desirable.

@existentialism
Copy link
Contributor

Won't this break for any Windows user trying to run npm start?

@gaearon
Copy link
Contributor

gaearon commented Nov 22, 2016

I don't think so, there's a try/catch there.

@existentialism
Copy link
Contributor

existentialism commented Nov 22, 2016

👍

Might be nice to avoid this output though:

image

@fson
Copy link
Contributor

fson commented Nov 22, 2016

We could probably fix that by ignoring the error stream (stderr) from the child process. execSync takes the stdio option that can be used to control that.

@gaearon
Copy link
Contributor

gaearon commented Nov 22, 2016

@existentialism FYI, you should get an invitation to be a repo collaborator for your PRs and help reviewing changes.

@ianmcnally
Copy link
Contributor Author

ianmcnally commented Nov 22, 2016

CI is failing with the error:

Module not found: Error: Cannot resolve module 'react' in /tmp/tmp.oNXJvttoVP/test-app/src

Anyone seen that before?

Update: look like some flakiness in the build. Back to green!

- Make sure any potential errors don't leak to the user
@ianmcnally
Copy link
Contributor Author

ianmcnally commented Nov 22, 2016

Added ignoring of stderr in 03122b5. Tested in with a bogus shell command and got no error output. Great find again, @fson!

@fson fson merged commit 05c3b55 into facebook:master Nov 22, 2016
@fson
Copy link
Contributor

fson commented Nov 22, 2016

Nice work, thank you @ianmcnally! 👍

@existentialism
Copy link
Contributor

@ianmcnally nice work!

@gaearon @fson happy to contribute!

jarlef pushed a commit to jarlef/create-react-app that referenced this pull request Nov 28, 2016
* add logging of existing port process on start

* Move port process wording in start command on to next line

* Color the named processes as cyan in terminal output

* Add handling for multiple processes on a part

- With the currently process filtering, if multiple processes are returned as running on port 3000, this command would fail. This splits apart the process IDing and the process naming, to support multiple processes.
- One curious thing about the bash command to get processes, is that it'll include browsers with a window open on localhost:3000. May want to reconsider that.

* Add process directory to existing port warning

- also moved terminal coloring up, when getting the process, to be able to distinguish the process command from the directory

* Change output color to all cyan, except "in"

* Rename getProcessNameOnPort -> getProcessForPort

- better reflects its broadened scope (both command and directory)

* Add checking if process is a CRA instance, to customize port running message

- moved from using package.json to a regex, for reliability

* Move getProcessForPort to react-dev-utils

- also allowed for breakdown of commands into helper methods

* Add documentation for getProcessForPort

* Add getProcessForPort to list of dev-scripts files

* Use app's package name when CRA app is running on another port

* Filter port process by those listening

- Removed the handling of multiple process IDs since you can filtering by listening process (and not have the browser in the list of processes)
- Trimmed the terminal outputs for better matching (process id) and better terminal output (directory of process)

* Update README on port helpers, to specify only one port returned

* Add ignore of stderr when executing process commands

- Make sure any potential errors don't leak to the user
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
* add logging of existing port process on start

* Move port process wording in start command on to next line

* Color the named processes as cyan in terminal output

* Add handling for multiple processes on a part

- With the currently process filtering, if multiple processes are returned as running on port 3000, this command would fail. This splits apart the process IDing and the process naming, to support multiple processes.
- One curious thing about the bash command to get processes, is that it'll include browsers with a window open on localhost:3000. May want to reconsider that.

* Add process directory to existing port warning

- also moved terminal coloring up, when getting the process, to be able to distinguish the process command from the directory

* Change output color to all cyan, except "in"

* Rename getProcessNameOnPort -> getProcessForPort

- better reflects its broadened scope (both command and directory)

* Add checking if process is a CRA instance, to customize port running message

- moved from using package.json to a regex, for reliability

* Move getProcessForPort to react-dev-utils

- also allowed for breakdown of commands into helper methods

* Add documentation for getProcessForPort

* Add getProcessForPort to list of dev-scripts files

* Use app's package name when CRA app is running on another port

* Filter port process by those listening

- Removed the handling of multiple process IDs since you can filtering by listening process (and not have the browser in the list of processes)
- Trimmed the terminal outputs for better matching (process id) and better terminal output (directory of process)

* Update README on port helpers, to specify only one port returned

* Add ignore of stderr when executing process commands

- Make sure any potential errors don't leak to the user
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
* add logging of existing port process on start

* Move port process wording in start command on to next line

* Color the named processes as cyan in terminal output

* Add handling for multiple processes on a part

- With the currently process filtering, if multiple processes are returned as running on port 3000, this command would fail. This splits apart the process IDing and the process naming, to support multiple processes.
- One curious thing about the bash command to get processes, is that it'll include browsers with a window open on localhost:3000. May want to reconsider that.

* Add process directory to existing port warning

- also moved terminal coloring up, when getting the process, to be able to distinguish the process command from the directory

* Change output color to all cyan, except "in"

* Rename getProcessNameOnPort -> getProcessForPort

- better reflects its broadened scope (both command and directory)

* Add checking if process is a CRA instance, to customize port running message

- moved from using package.json to a regex, for reliability

* Move getProcessForPort to react-dev-utils

- also allowed for breakdown of commands into helper methods

* Add documentation for getProcessForPort

* Add getProcessForPort to list of dev-scripts files

* Use app's package name when CRA app is running on another port

* Filter port process by those listening

- Removed the handling of multiple process IDs since you can filtering by listening process (and not have the browser in the list of processes)
- Trimmed the terminal outputs for better matching (process id) and better terminal output (directory of process)

* Update README on port helpers, to specify only one port returned

* Add ignore of stderr when executing process commands

- Make sure any potential errors don't leak to the user
@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.

5 participants