Avoid breaking existing consumers who use startPort >= 40000

[BarthesSimpson]

This would fix the issue identified in #85. If there is an existing consumer who uses getPort with a startPort greater than 40000, instead of throwing an Error this will log a warning to the console and bump the stopPort up to 65535 (i.e. behave the same as before exports.highestPort was reduced to 40000).

An error is still thrown if the stopPort is lower than the stopPort when both of these values are below 40000.

[BarthesSimpson]

Updated to also address #84. By default, getPort will use the previous sequential scan. If useRandom: true is passed inside the options, then the random search will be used instead.

[jaredjj3]

LGTM

Thank you for working on this PR.

[jelhan]

Looks good and seems to fix both issues encountered in Ember CLI.

[jelhan]

I see some strange behavior for port = 40000. I'm not 100%ly sure yet if this is a bug or just coincidence. If calling ember serve --port 40000, which should equal getPort({ port: 40000 }) I'm seeing No open ports found in between 40000 and 40000. It's working fine for 39999 and 40001. Haven't have the time yet to follow the investigate the control flow for that edge case yet.

[BarthesSimpson]

I think that's because the startPort:stopPort interval is closed, so if startPort and stopPort are the same, no port will be checked. Let me double check that the interval is supposed to be open and then I can update this PR accordingly (later today).

[BarthesSimpson]

OK, yeah the interval should be open, not closed. Will update now.

[haoqunjiang]

Ping @eriktrom. Sorry to bother you, but is there any chance to get this PR merged soon? Lots of downstream repositories are relying on this feature to work correctly.

[Ten0]

Ping @eriktrom. Sorry to bother you, but is there any chance to get this PR merged soon? Lots of downstream repositories are relying on this feature to work correctly.

+1. With 877 dependant packages and 4 685 655 weekly downloads, this as-patch breaking change that prevents port choice is breaking so many systems right now and shouldn't stay broken for so long.

[soloitguy]

Downgrading to 1.0.21 explicitly is a temporary fix. Our outage was a bit too long to wait for the PR.

[eriktrom]

@BarthesSimpson - thanks - i will take a closer look tonight, tomorrow and follow up.

For today, i rolled back to v1.0.21 (as v1.0.23)

[eriktrom]

@BarthesSimpson - I feel like we should consider an option that does not directly modify the main repo -- maybe another export getRandomPort which uses the in place randomization (port 0 actually may do this automatically, it does in ember-cli, it's been years but I believe its a straight passthrough to portfinder port: 0 or port: undefined.

I'm very hesitant to release another version that is not a semver major bump unless we can guarantee that changes are strictly additive.

Thoughts?

[eriktrom]

If useRandom: true is passed inside the options, then the random search will be used instead.

The only issue here is that, for example, lets say you start 2 test servers inside ember cli - if we pass useRandom: true then we have detect the os arch to be darwin - which seems reasonable - unless via docker or minikube your actually using hyperkit - with port forwarding - to be more clear on the port 40,000-60,000 thing - the error is a permission error - binding to the socket works, using it throws a permission error

b/c of matrix of use cases upstream are very high, this single example should help explain my concern.

fixing for ember-cli is one thing, for firebase, circleci and other tools that depend on the current behavior 'as is' -- i think we make a new method or (if the code was cleaner) an extension poiint, but new method more likely in this case

[BarthesSimpson]

Sure - I'll take another look this weekend and see if I can find a good approach for adding a getRandomPort export.

[eriktrom]

@BarthesSimpson - see #91 for thoughts on releasing changes to portfinder moving forward - mostly that we should only do so if we do a semver major release, reducing exposure to how this library has been used by different consumers over the past many years.

If we go that route, we can consider adding deprecation warning to v1.x that would not take effect until v3.x - although someone would need to take the lead on this as I'm involved in the depths of a few other topics right now - just fyi - open a meta ticket if you want to start a discussion outside this thread regarding such things

I'm going to close this issue and link to it from a clean issue, so we don't spam

[eriktrom]

@BarthesSimpson - #92 is a meta issue on a semver major that I would feel comfortable releasing these changes to. Re-opening, with 2.x tag.

[eriktrom]

FYI: if interest remains in merging this (w/ the randomization feature via @BarthesSimpson original work) see #105 and vote. I'm taking a quick pulse of the traction it would garner if made possible to you and others.

[eriktrom]

closing to cleanup issue list, comments or questions still welcome, if needed

consolidated via #99