[Playground CLI] Return a readable error and hint when given port is already in use

[mho22]

Motivation for the change, related issues

Based on this issue

The goal was to ignore stack traces, only print error messages in the CLI output style.

Implementation details

Added a onError option in ServerOptions that will be called once a error happens in app.listen(..).

Testing Instructions

CLI

or

Run the following command in two separate terminals :

node --no-warnings --loader=./packages/meta/src/node-es-module-loader/loader.mts ./packages/playground/cli/src/cli.ts server

WordPress Playground CLI

PHP 8.3  WordPress latest
Extensions intl

Running Blueprint 100%

Ready! WordPress is running on http://127.0.0.1:9400 (1 worker)

WordPress Playground CLI

PHP 8.3  WordPress latest
Extensions intl

Error: listen EADDRINUSE: address already in use :::9400

[fellyph]

I would like to have some guidance for users who are not familiar with the CLI yet, showing the flag --port as an alternative.

Can we set a message like:

Error: listen EADDRINUSE: address already in use :::9400. 

Use the --port flag to specify a different port. 

What do you think, @brandonpayton?

[mho22]

@fellyph @brandonpayton What do you think ?

I’m not convinced about the newLine parameter :

printInfo(message: string, newLine = false): void {
	if (this.isQuiet) return;

	if (newLine) this.writeStream.write('\n');

	this.writeStream.write(`${this.cyan('Info:')} ${message}\n`);
}

Do you have any insights on it?

[mho22]

I also prepared code to face these use cases :

If no port is mentioned and port 9400 is already in use, we run on a random free port. No error, hint or stack trace returned.
If a port is mentioned and that port is already in use, we return the error and hint without stack trace.

I'll create a new pull request with it once this one is merged.

[brandonpayton]

I also prepared code to face these use cases :

If no port is mentioned and port 9400 is already in use, we run on a random free port. No error, hint or stack trace returned.
If a port is mentioned and that port is already in use, we return the error and hint without stack trace.

I'll create a new pull request with it once this one is merged.

@mho22 I think this is a good idea that could just be done in this PR. If we do this, we won't even need to tell users about the --port option because we only fail on a busy port when the --port option was used.

[brandonpayton]

I’m not convinced about the newLine parameter :

I'm not convinced either. It seems to make sense that you'd only want to print an extra newline char when the info message itself will be printed, but it feels a bit entangled to have the function ask "Should I print an extra newline?"

Maybe printing info in this PR will be going away anyway, if we always auto-select an available port when --port is not specified.

[mho22]

@brandonpayton I removed the printInfo and implemented a isPortInUse function. I think we're good.

[brandonpayton]

@brandonpayton I removed the printInfo and implemented a isPortInUse function. I think we're good.

@mho22 It looks like this PR will pick a random available port when the --port arg is included and the specified port is unavailable. Is that right? Since there is a default 9400 value for the port yargs declaration, I don't currently see a way to tell whether or not the port arg was provided.

What we want is the following:

If no port is mentioned and port 9400 is already in use, we run on a random free port. No error, hint or stack trace returned.
If a port is mentioned and that port is already in use, we return the error and hint without stack trace.

[mho22]

This is probably not very clear, I admit.

const selectedPort = args.command === 'server' ? (args.port ?? 9400) : 0;

...

port: args.port
	? args.port
	: !(await isPortInUse(selectedPort))
		? selectedPort
		: 0,
onError: (error: NodeJS.ErrnoException) =>
	cliOutput.printError(error.message),

If there's a args.port then use it [ and if it is already in use it will fail and return an error without stack trace ], else if the selectedPort is already in use, use port 0 [ random one in this case ].

[brandonpayton]

If there's a args.port then use it [ and if it is already in use it will fail and return an error without stack trace ], else if the selectedPort is already in use, use port 0 [ random one in this case ].

@mho22 Since we declare a default value of 9400 in the yargs declarations, won't yargs always assign to args.port? I might be missing something, but if not...

It would probably be good to add a couple of run-cli test for this, using parseOptionsAndRunCLI() which exercises yargs and will confirm the behaviors.

[mho22]

@brandonpayton I had to remove the default: 9400 to make this work.

It would probably be good to add a couple of run-cli test for this, using parseOptionsAndRunCLI() which exercises yargs and will confirm the behaviors.

On it.

[mho22]

@brandonpayton I've added tests and they ran successfully. 👍.

[brandonpayton]

Thanks, @mho22! This looks good.

Before merging, would you please update the --port args declared descriptions to mention the port defaults to 9400 when available? It seems like it would be good to still document this, even though we had to remove the yargs default.

[fellyph]

@mho22, this check also works for npx @wp-playground/cli start ?

[mho22]

@fellyph I tried the following command and it worked as expected :

node --no-warnings --loader=./packages/meta/src/node-es-module-loader/loader.mts ./packages/playground/cli/src/cli.ts start