Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
filiphazardous opened this issue Oct 16, 2023 · 3 comments · Fixed by #72
Closed

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

filiphazardous opened this issue Oct 16, 2023 · 3 comments · Fixed by #72

Comments

@filiphazardous
Copy link
Contributor

filiphazardous commented Oct 16, 2023

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
Copy link

peterroe commented Oct 22, 2023

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

@filiphazardous
Copy link
Contributor Author

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
Copy link
Member

pi0 commented Oct 23, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants