feat: Port in use checking for development server

[Josh-Walker-GM]

Fixes #4697

Problem
When starting more than one development server via yarn rw dev the second instance kills the first instances web server.

Solution
The yarn rw dev command now checks if the api or web port is in use and if so warns the user and informs them of the the next available port(s).

Outstanding

• Needs tests?

[Josh-Walker-GM]

I'm not entirely sure of how to propagate the available port option when a user is happy to do so. One idea is to update the value of the forward to add or override the port option? This is how it is currently done.

Problem: Could not get the port used by the api server to be recognised as in use even with using portfinder lib. This means if a user runs a second dev server the suggested port conflicts with the previous dev server api port so there are still issues.

Still not sure if the same duplicate port avoidance check is required with the api server - this seems tricker to implement given the current setup.

[jtoar]

Hey @Josh-Walker-GM thanks for taking this on! Just had a first pass left a few comments.

Tried it out once and seems like it kinda works, so great for a first implementation. I had another rw app running to simulate, and it suggested 8911, which is what the api side is using, so there may need to be a few tweaks still. I'll do another pass after I circle around to a few other PRs, but looks promising so far. Maybe we cant abstract this into a function when we're done just to keep things readable.

[Josh-Walker-GM]

Thanks for taking a look.

Definitely not quite there in solving the problem I agree. I had noticed this issue around suggesting the API server port and had documented it on the PR. It's a strange one that it can't seem to recognise the API port is in use but manages to notice the web port is in use. Part of me maybe wonders if it's to do with the :: host that's used by default - this is just my suspicion but I'm not a networking guru so I could be barking up the wrong tree. When I get a chance I'll test out my suspicions.

Very happy to extract out into a function once the functionality is there. Didn't want to get ahead of myself before it was fully working and because I'm still getting accustomed to the project 😄

[Josh-Walker-GM]

@jtoar Can I ask what platform you tested on? You wouldn't happen to have been using WSL?

[Josh-Walker-GM]

Okay I'll provide an update here as to what fixed the outstanding issues and I'll update the original PR description to give the latest summary.

I realised that the shutdownPort was being called with the default values whenever you called yarn rw dev. So we were shutting down the existing API server before we checked ports so of course it was suddenly available.

Changes I implemented as a result:

1. Update the devHandler to shutdown whatever port is used not just the default
2. Check both api and web server ports to ensure they aren't in use. If they are prompt for next available port
3. Added port option to the watch-api-server

[Josh-Walker-GM]

Tests are failing because we can no longer run multiple dev servers in parallel as they seek input given the ports will be in use. if someone is able to jump in and serialise the tests I would be very grateful. After that I would say this would be ready for review.

EDIT:
The tests actually fail for a different reason. I updated the inline snapshot to include the required port option. However, the remaining failing test expects a value much different from what it receives. Someone with greater experience will have to debug that particular test, sorry.

[jtoar]

Hey @Josh-Walker-GM, thanks for waiting! I have another round. I think we're nearing the end, but I need to confirm something about how the web side proxies requests to the api side.

A lot of the comments are just improving the copy in the console logs. But I did come up with a fix for extracting the port from the --fwd flag. Let me know what you think, and I'll report back about the proxy thing when I get my head around it.

[jtoar]

Since webpack uses the last port, we can just concat it on if it's different. (I also moved this up a block like apiPort = freePort)

Suggested change

	}
	webPort = freePort
	forward = forwardedPortSet
	? forward.replace(forwardedPortMatches[0], ` --port=${freePort}`)
	: forward.concat(` --port=${freePort}`)
	webPort = freePort
	forward = forward.concat(` --port=${freePort}`)
	}

[Josh-Walker-GM]

Happy with this too. I added a quick comment just to let anyone know the expected behaviour with multiple ports in the forward string. I also added a little check so we only concat on an additional port if it's different from the one they originally specified (via config or forward).

// If needed add the newly chosen port to the webpack forward string. Note: webpack will use the final --port option if more than one are supplied
if (webPort !== freePort) {
  forward = forward.concat(` --port=${freePort}`)
}
webPort = freePort

Hope this is fine?

[Josh-Walker-GM]

Okay I've changed my mind... I think we should just show the user an error and tell them which ports could be used instead. We shouldn't try and do it for them.

[jtoar]

@Josh-Walker-GM I'll look into the smoke-test when I have time; we change a few ports there

[Josh-Walker-GM]

Nevermind - I was barking up the wrong tree.

UPDATE: GQL error: Cannot query field "timestamp" on type "Post" Seems to be the relevant error.

UPDATE: I had a passing smoke test which contained the expected createdAt in the BlogPostsCell.tsx file and then after running some more smoke tests I have observed it being changed to timestamp...

UPDATE: I am beginning to think that the smoke tests relied on this issue to work in the first place. It was using the fact that subsequent server instances killed the first and run themselves.

[dthyresson]

FYI, I had a case where a dev server on port 891 didn't exit cleanly and it didn't appear as a process to kill.

But, I could find it on OSX via:

sudo lsof -P -i TCP -s TCP:LISTEN

And then killing the PID associated with the 8910 port.