refactor: using indexOf to check unique host

[2kjiejie]

Hi,

I was reading the source code for personal interest and found that you are using a loop to check if the host is unique. I think indexOf() could be much faster than the loop. Here is the comparison on my laptop:

loop: 4.363ms indexOf: 0.005ms

[eriktrom]

I will take a look - you correct in one sense - perf - I would have to look at it closely to ensure correctness - if u can summarize without me spending too much time verifying that no breaking changes would occur, i'd actually be open to this idea.

keep in mind, the code is by definition legacy - meaning its stable but changes are extremely hard to make with 100% certainty and this package is a dependency for so many other packages, including most dev servers that live behind most modern frameworks (like create-react-app -> webpack dev hot reload server(not the official name but u get the gist)...

so yep, nice find, but can we ensure it wont ruin everyone's day or weekend or circle CI or ... the list is long, I broke it once, many years ago and it was a huge mistake then, if it breaks anything that depends on it today, the effects on teams and github orgs would be an order of magnitude worse given the number of downloads for this package has skyrocketed since then.

tldr - we can't be anything but completely certain if we do make a change...

convince me that won't happen with a short proof of correctness and I'll match your efforts to make it happen :)

(and yes, i realize my request sounds insane given its very localized and very few lines, but that's how it is with this particular library - its like a siren calling ur name, it lures you in and then gives a hand grenade as a parting gift)

[2kjiejie]

Thanks for the reply! I fully understand what you are concerned about, so I will try to convince you through functionality and compatibility.

Functionality

Here is the source code of indexOf:

Array.prototype.indexOf = (function(Object, max, min) {
    "use strict"
    return function indexOf(member, fromIndex) {
      if (this === null || this === undefined)
        throw TypeError("Array.prototype.indexOf called on null or undefined")

      var that = Object(this), Len = that.length >>> 0, i = min(fromIndex | 0, Len)
      if (i < 0) i = max(0, Len + i)
      else if (i >= Len) return -1

      if (member === void 0) {        // undefined
        for (; i !== Len; ++i) if (that[i] === void 0 && i in that) return i
      } else if (member !== member) { // NaN
        return -1 // Since NaN !== NaN, it will never be found. Fast-path it.
      } else                          // all else
        for (; i !== Len; ++i) if (that[i] === member) return i

      return -1 // if the value was not found, then return -1
    }
  })(Object, Math.max, Math.min)

You can find it is very similar with your function and because it is a build-in function, it has better performance.

Since _defaultHosts is a non-empty array, what we need to care about is the type of option.host. So I have tried the circumstances below (inspired by https://github.com/v8/v8/blob/dc712da548c7fb433caed56af9a021d964952728/test/mjsunit/array-indexing-receiver.js):

1. typeof(option.host) is string
2. typeof(option.host) is number
3. typeof(option.host) is object (includes Object, Array, Map, null)
4. typeof(option.host) is boolean
5. typeof(option.host) is bigint
6. typeof(option.host) is undefined

The result of the new function are always the same as the formal function.

Compatibility

The only risk is from indexOf. As far as I know, indexOf() was added to the ECMA-262 standard in the 5th edition. So for 100% compatibility, we can apply polyfill like this:

if (Array.prototype.indexOf) {
      if (exports._defaultHosts.indexOf(options.host) != -1) {
        exports._defaultHosts.push(options.host);
      }
    }
    else {
      var hasUserGivenHost = false;
      for (var i = 0; i < exports._defaultHosts.length; i++) {
        if (exports._defaultHosts[i] === options.host) {
          hasUserGivenHost = true;
          break;
        }
      }

      if (!hasUserGivenHost) {
        exports._defaultHosts.push(options.host);
      }
    }

That's all the things I have done. Hopefully it can convince you the correctness of this PR. If you want more test, please feel free to contact me.

[eriktrom]

wow - exceptional work - it's even educational, adding value to the project beyond the code itself. Thank you.

I will be back, very likely with a merge, I'll make it a priority, thanks again

[2kjiejie]

You're welcome. Really appreciate your work, so it would be nice if I can contribute.

[eriktrom]

thanks and sorry for the delay

[yuningjiang123]

I wonder if there's sth wrong with this pr

if (exports._defaultHosts.indexOf(options.host) !== -1) { exports._defaultHosts.push(options.host) }

Shouldn't it be:

if (exports._defaultHosts.indexOf(options.host) === -1) { exports._defaultHosts.push(options.host) }

options.host should be appended to _defaultHosts only when it is not inside _defaultHosts.

[2kjiejie]

Yes, you are right. @eriktrom Would you mind considering fixing it in the most recent version? I sincerely apologize for that.

[MasterOdin]

I created https://github.com/http-party/node-portfinder/pull/148 to fix the bug, though I'm not sure how often @eriktrom checks this repo unfortunately to get it merged.

[eriktrom]

Apologies for the delay.

I did normally manually check every 6 months until the recent GitHub hack in which I installed the GitHub app.

I'm here and will check the code on my computer by this evening.

Cheers.