Discussion: portfinder v2, foster community, docs and general guidelines

[eriktrom]

This issue serves to document features requested by community members, and how we should manage such needs.

tl;dr - this lib at v1 is largely frozen, code wise. It needs major help though with communication of it's intent and guidelines for extending it for individual use cases.

The following outlines those current needs. At the bottom I've added some notes about v2, however unlikely it is to happen. (I'd rather call into this lib than change its implementation as well, if possible for the last bullet under v2 section, meaning contributors should consider this the norm when thinking about their use individual use cases, given I'm thinking about it for the general use case mentioned in that bullet, due to minut edge cases currently encoded into the logic of the current stable release. FYI - i'm not rejecting ideas, just working on how to allow them by extension, not modification, of this lib. Ideas welcome).

Current Needs:

• write docs, including emphasis on development port finding, stability over speed, and stability over features that don't serve the larger portion of the consumer base (aka, wrap this lib if you want to extend it, provide examples and how to's for those in need).

• Foster community involvement, be kind, but push back, link pr's that don't follow above guidelines to the docs that (need to be written) on how to wrap this library for single purpose needs. Example: random ports could be passed in to this lib from a wrapper lib that deals with which random port to check, leaving portfinder simple, encapsulated and stable (cc @BarthesSimpson I think this is your best option for Avoid breaking existing consumers who use startPort >= 40000 #86)

• Add changelog and release tool (it should give credit to author, provide link to the pr merged, wiith description (ideally pr title, which means the only thing we need to manually ensure is that PR titles are accurate descriptions prior to merge) (Before filing a socket not closed issue: understanding async I/O + ordering + OS differences + stability requirements #99)

We don't have a code of conduct, free speech welcome, but derogatory comments towards individuals (not ideas) will not be tolerated.

• Make a code of conduct file, follow community norms (ideally, I like the way rust has handled this, see. Copy their statement or similar). [Good for new contributor] (issue Create code of conduct #98)

V2

The following features would be released in a new semver major version of portfinder, 2.x.

Note: v2 is not likely, but here is what would go into it, bullet 3, CATALYST, being the most important, if needed, it is the only reason to pursue a new major version)

• Use neo-async: chore(): update deps #91 EDIT: upgraded to async v2.

  • In general don't use new libraries due to security, with maybe the exception someday of a new testing library (which is not included in dist npm package anyway).

• Use randomized ports: Avoid breaking existing consumers who use startPort >= 40000 #86 EDIT: this changes the behavior, and breaks the rule of refactoring in additive changes, aka, "underlying changes to implementation of a consumer facing <app|lib> should not change it observed behavior. The change that this introduces is 2 fold:

  1. ports are randomized - there may be consumers that don't expect this, example: ember-cli's ember s when run in 2 terminals, is expected to increase the 2nd applications test port sequentially -- for many, myself included, I have bookmarks for 7200, 7202, etc on mobile devices that reload and run the tests on each device for each app, especially when an addon is involved(which when changed, runs the tests for the yarn linked ember addon, and the consuming app that uses the yarn linked dep I'm working on.

  2. This can be observed without that explanation in the tests for Avoid breaking existing consumers who use startPort >= 40000 #86, which was reverted due to a minor but far reaching error in when to terminate the algo it updated. (on that note, the tests are cleaner, they just changed the behavior, something I was nervous about when merging. In theory, randomizing should not matter, but in practice people may depend on such subtle bugs, which over time, become 'the outward behavior of the code, as perceived by the consumer, whether a user or a developer of a downstream codebase'. (quoting martin fowler, kent beck, in a hurry, forgive where mis-quoted).

• CATALYST: use echo server to ensure port is not being used by a privileged user or group (I tried this couple years ago, most obvious breaking change was when it bound to bluetooth and/or mac os touch bar ports, both owned by root:wheel. Node docs do have privileged error code (as ERR* constant). I was trying to avoid checking every possible error and basically catch all but the ones we currently catch(if that), iterate to next port regardless of what that error was. Also, bound the range to be > 1024 and < 40000(apple uses this for touch bar, and you'd be surprised how often people hit that when using ember-cli in practice - they had to restart their computer or pass cli option for a lower port to iterate from for use with live reload in ember-cli. I changed the default to 7020 as a result, thus this issue exists, but hidden, meaning until substantial bug reports come in, 2.x is not a priority, as this is the single feature/bug fix that really requires changing an otherwise stable code base.

---

---

TODO: this list was written fast, it may lack clarity, please ask or comment where confused. I'll probably clean it up in later, but wanted to braindump while I was in the neighborhood. Related to http-party/node-http-proxy#1383 - and should reflect the tersness of that bullet point list ideally. ([but] "time keeps on ticking, ticking, ticking into the future.... 🎵" )

[eriktrom]

superseded by #105 (comment)
superseded by #99