elderjs icon indicating copy to clipboard operation
elderjs copied to clipboard

[Dev Mode] if port 3000 is already in use, fallback to random port

Open kinooyume opened this issue 4 years ago • 5 comments

kinooyume avatar Mar 11 '21 10:03 kinooyume

Maybe fixed?

timeshift92 avatar Jun 21 '21 15:06 timeshift92

What is the expectation if that random port is taken? Keep trying to find a random port or make the console log messages provide a way for the user to know what to do?

In the server.js the following lines exist:

const SERVER_PORT = process.env.SERVER_PORT || 3000;
server.listen(SERVER_PORT, (err) => {
  if (err) {
    console.log(err);
  }
  console.log(`> Elder.js running on http://localhost:${SERVER_PORT}`);
});

You can pass in a port if you know 3000 is used or change the default. This is a standard in most development languages to set a sane default and take a way for developers to pass in a new port on runtime as they may be working on multiple projects.

You could do it like this if you want to achieve this:

const SERVER_PORT = process.env.SERVER_PORT || 3000;
const start = (port = SERVER_PORT) => {
    server.listen(port, (err) => {
        if (err) {
            console.log(err)
        }
    })
    if (server.server.listening) {
        console.log(`> Elder.js running on http://localhost:${port}`);
    }
}
start()
let startAttempts = 0;
process.on('uncaughtException', (err) => {
    if (err.code === 'EADDRINUSE') {
        start(SERVER_PORT + startAttempts++)
    }
})

Unsure if that is what you are looking for but also unsure why this is considered an enhancement considering its quite a bit of extra code to solve a problem of a developer not paying attention or not giving more information on the error for new users.

If you are expecting that the information for the error message to be more clear and give the user an idea what the issue is then I would suggest the edition of the following code instead of the re-bind logic above

process.on('uncaughtException', (err) => {
    if (err.code === 'EADDRINUSE') {
        console.log(`\n
\tFailed to startup due to the port (${SERVER_PORT}) being in use by another process. 
\tTo remedy do one of the following:
\t\tChange the default port in the server.js file 
\t\tset your environment variable for SERVER_PORT to an available port
\n`)
    }
})

If you want either of these to be chosen then I can submit a PR with the preference.

@timeshift92 - it is not fixed as of template version 1.3.2 (which uses "@elderjs/elderjs": "^1.5.5", )

ioneyed avatar Jul 26 '21 02:07 ioneyed

A good start here would be to choose a default port other than 3000 — that's commonly in use on my machines.

markjaquith avatar Aug 11 '21 01:08 markjaquith

The solution suggested by @ioneyed seems reasonable:

start()
let startAttempts = 0;                   <==== should probably be 1
process.on('uncaughtException', (err) => {
    if (err.code === 'EADDRINUSE') {
        start(SERVER_PORT + startAttempts++)
    }
})

trasherdk avatar Aug 13 '21 09:08 trasherdk

Listening on port 0 actually assigns you a random free port, you could just do that if the port is taken Source: https://stackoverflow.com/questions/28050171/nodejs-random-free-tcp-ports

GrahamSH-LLK avatar Sep 09 '21 20:09 GrahamSH-LLK