add logging of existing default port process on start #816

ianmcnally · 2016-10-01T18:31:46Z

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

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

It shows the command name and directory.

without a process found

It shows the original, generic message.

facebook-github-bot · 2016-10-01T18:33:08Z

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!

facebook-github-bot · 2016-10-01T18:53:45Z

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

gaearon · 2016-10-01T22:29:05Z

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 · 2016-10-02T21:01:39Z

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

ianmcnally · 2016-10-02T21:09:06Z

@gaearon in dc824f5, it now reads:

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

gaearon · 2016-10-03T12:59:19Z

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.

- 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 · 2016-10-04T14:24:20Z

Hey Dan,

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

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 · 2016-10-06T03:03:37Z

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 · 2016-10-06T15:06:48Z

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

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 ?

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

ianmcnally · 2016-10-06T22:25:06Z

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.,

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

ianmcnally · 2016-10-09T22:13:33Z

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

ianmcnally · 2016-10-10T15:18:06Z

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

gaearon · 2016-10-11T11:44:39Z

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?

fson · 2016-11-22T10:42:46Z

I tested starting the dev server for a CRA project and then trying to start another one, got this message:

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:

Maybe we can look for the string LISTEN to narrow down the list of processes?

fson · 2016-11-22T10:47:02Z

Looks like we can pass the -sTCP:LISTEN option to lsof to only get processes listening on the port:

fson · 2016-11-22T11:01:02Z

packages/react-dev-utils/getProcessForPort.js

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


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

Otherwise looks great.

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

Do we need to detect/support Windows here?

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.

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 · 2016-11-22T16:58:00Z

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:

which looks desirable.

existentialism · 2016-11-22T17:03:50Z

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

gaearon · 2016-11-22T17:09:42Z

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

existentialism · 2016-11-22T17:10:08Z

👍

Might be nice to avoid this output though:

fson · 2016-11-22T18:11:01Z

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 · 2016-11-22T19:18:52Z

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

ianmcnally · 2016-11-22T19:20:18Z

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 · 2016-11-22T19:24:57Z

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

fson · 2016-11-22T20:44:45Z

Nice work, thank you @ianmcnally! 👍

existentialism · 2016-11-23T01:05:43Z

@ianmcnally nice work!

@gaearon @fson happy to contribute!

* 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

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 added the CLA Signed label Oct 1, 2016

add logging of existing port process on start

626c8d0

ianmcnally force-pushed the guess-existing-port-process-441 branch from 965a68c to 626c8d0 Compare October 1, 2016 18:54

facebook-github-bot added the CLA Signed label Oct 1, 2016

Move port process wording in start command on to next line

dc824f5

facebook-github-bot added the CLA Signed label Oct 2, 2016

Ian McNally added 2 commits October 4, 2016 10:18

Color the named processes as cyan in terminal output

abeca8a

facebook-github-bot added the CLA Signed label Oct 6, 2016

Add process directory to existing port warning

e8d0053

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

ianmcnally force-pushed the guess-existing-port-process-441 branch from 504efb6 to e8d0053 Compare October 6, 2016 15:33

facebook-github-bot added the CLA Signed label Oct 6, 2016

Change output color to all cyan, except "in"

e3dcaaf

Rename getProcessNameOnPort -> getProcessForPort

9c2e3aa

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

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

facebook-github-bot added the CLA Signed label Oct 10, 2016

gaearon added this to the 0.8.0 milestone Nov 20, 2016

fson reviewed Nov 22, 2016

View reviewed changes

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

e25f113

Add ignore of stderr when executing process commands

03122b5

- Make sure any potential errors don't leak to the user

fson merged commit 05c3b55 into facebook:master Nov 22, 2016

ianmcnally deleted the guess-existing-port-process-441 branch November 22, 2016 21:14

ianmcnally mentioned this pull request Nov 22, 2016

Guess what app is already running in port 3000 #441

Closed

KyleAMathews mentioned this pull request Nov 22, 2016

Log which process is using port when there's a conflict gatsbyjs/gatsby#569

Closed

fson added tag: new feature tag: enhancement and removed tag: new feature labels Dec 1, 2016

andriijas mentioned this pull request Jan 23, 2018

Update to webpack 4 and integrate automatic vendor code splitting #3878

Closed

lock bot locked and limited conversation to collaborators Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add logging of existing default port process on start #816

add logging of existing default port process on start #816

ianmcnally commented Oct 1, 2016 •

edited

Loading

facebook-github-bot commented Oct 1, 2016

facebook-github-bot commented Oct 1, 2016

gaearon commented Oct 1, 2016

ianmcnally commented Oct 2, 2016

ianmcnally commented Oct 2, 2016

gaearon commented Oct 3, 2016

ianmcnally commented Oct 4, 2016 •

edited

Loading

ianmcnally commented Oct 6, 2016

ianmcnally commented Oct 6, 2016 •

edited

Loading

ianmcnally commented Oct 6, 2016

ianmcnally commented Oct 9, 2016

ianmcnally commented Oct 10, 2016 •

edited

Loading

gaearon commented Oct 11, 2016

fson commented Nov 22, 2016 •

edited

Loading

fson commented Nov 22, 2016

fson Nov 22, 2016

ianmcnally Nov 22, 2016

existentialism Nov 22, 2016

gaearon Nov 22, 2016 •

edited

Loading

existentialism Nov 22, 2016

ianmcnally commented Nov 22, 2016 •

edited

Loading

existentialism commented Nov 22, 2016

gaearon commented Nov 22, 2016

existentialism commented Nov 22, 2016 •

edited

Loading

fson commented Nov 22, 2016

gaearon commented Nov 22, 2016

ianmcnally commented Nov 22, 2016 •

edited

Loading

ianmcnally commented Nov 22, 2016 •

edited

Loading

fson commented Nov 22, 2016

existentialism commented Nov 23, 2016

add logging of existing default port process on start #816

add logging of existing default port process on start #816

Conversation

ianmcnally commented Oct 1, 2016 • edited Loading

with a CRA app process found

with a non-CRA process found

without a process found

facebook-github-bot commented Oct 1, 2016

facebook-github-bot commented Oct 1, 2016

gaearon commented Oct 1, 2016

ianmcnally commented Oct 2, 2016

ianmcnally commented Oct 2, 2016

gaearon commented Oct 3, 2016

ianmcnally commented Oct 4, 2016 • edited Loading

ianmcnally commented Oct 6, 2016

ianmcnally commented Oct 6, 2016 • edited Loading

ianmcnally commented Oct 6, 2016

ianmcnally commented Oct 9, 2016

ianmcnally commented Oct 10, 2016 • edited Loading

gaearon commented Oct 11, 2016

fson commented Nov 22, 2016 • edited Loading

fson commented Nov 22, 2016

fson Nov 22, 2016

Choose a reason for hiding this comment

ianmcnally Nov 22, 2016

Choose a reason for hiding this comment

existentialism Nov 22, 2016

Choose a reason for hiding this comment

gaearon Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

existentialism Nov 22, 2016

Choose a reason for hiding this comment

ianmcnally commented Nov 22, 2016 • edited Loading

existentialism commented Nov 22, 2016

gaearon commented Nov 22, 2016

existentialism commented Nov 22, 2016 • edited Loading

fson commented Nov 22, 2016

gaearon commented Nov 22, 2016

ianmcnally commented Nov 22, 2016 • edited Loading

ianmcnally commented Nov 22, 2016 • edited Loading

fson commented Nov 22, 2016

existentialism commented Nov 23, 2016

ianmcnally commented Oct 1, 2016 •

edited

Loading

ianmcnally commented Oct 4, 2016 •

edited

Loading

ianmcnally commented Oct 6, 2016 •

edited

Loading

ianmcnally commented Oct 10, 2016 •

edited

Loading

fson commented Nov 22, 2016 •

edited

Loading

gaearon Nov 22, 2016 •

edited

Loading

ianmcnally commented Nov 22, 2016 •

edited

Loading

existentialism commented Nov 22, 2016 •

edited

Loading

ianmcnally commented Nov 22, 2016 •

edited

Loading

ianmcnally commented Nov 22, 2016 •

edited

Loading