unjs / get-port-please

🔌 Get an available open port
MIT License
245 stars 12 forks source link

options.port default to 3000, even in the presence of options.ports #67

Closed filiphazardous closed 8 months ago

filiphazardous commented 11 months ago

Environment

Node v18 (but I think it is universal)

Reproduction

The following repo gets the port 3000 unexpectedly: https://github.com/nuxt-modules/storybook/blob/main/src/storybook.ts (see line 11)

Here is the shortest snippet that reproduces the bug:

import { getPort } from 'get-port-please';
const port = getPort({ ports: [6006, 6007, 6008] });
if (port === 3000) {
  console.log('Port is unexpectedly returned as 3000');
}

Describe the bug

In the file get-ports.ts (https://github.com/unjs/get-port-please/blob/main/src/get-port.ts) on line 29 - the port option defaults to 3000 (unless otherwise defined in _userOptions). On line 53 in the same file, the same value is assigned a higher priority than the actually provided option ports. This makes the ports and portRange options hard to use.

If this behavior is intentional, it would be convenient if it was documented.

A suggested fix would be to add the default port to the end of portToCheck and unshift _userOptions.port to the front of the same array if it is provided.

Additional context

No response

Logs

This is a log generated by me putting `console.log` messages, outputting `_userOptions`, `options` and `availablePort` in the `index.mjs` file in `node_modules`. (feserver_1 being my docker container)

feserver_1  | _userOptions
feserver_1  |  {
feserver_1  |   "ports": [
feserver_1  |           443,
feserver_1  |           6007,
feserver_1  |           6008,
feserver_1  |           6009,
feserver_1  |           6010
feserver_1  |   ],
feserver_1  |   "verbose": true
feserver_1  | }
feserver_1  | options
feserver_1  |  {
feserver_1  |   "name": "default",
feserver_1  |   "random": false,
feserver_1  |   "ports": [
feserver_1  |           443,
feserver_1  |           6007,
feserver_1  |           6008,
feserver_1  |           6009,
feserver_1  |           6010
feserver_1  |   ],
feserver_1  |   "portRange": [],
feserver_1  |   "alternativePortRange": [
feserver_1  |           3000,
feserver_1  |           3100
feserver_1  |   ],
feserver_1  |   "verbose": true,
feserver_1  |   "port": 3000
feserver_1  | }
feserver_1  | availablePort:  3000
peterroe commented 11 months ago

Yes. It does seem a little counter-intuitive that there are times when we don't want the port as a first priority 👀

filiphazardous commented 11 months ago

Not sure what you mean. My case is that I have not defined port, but I have defined ports. In this case port defaults to 3000, and if that port is available - ports is disregarded.

If this is expected behavior, it should be documented that you can't use ports without also defining port in options.

pi0 commented 11 months ago

I think it makes sense to prefer custom port yrange when provided. Feel free to make a PR!