http-party / node-portfinder

A simple tool to find an open port or domain socket on the current machine
https://github.com/http-party/node-portfinder
Other
887 stars 95 forks source link

Add TS return signature to setPort functions #143

Closed MasterOdin closed 2 years ago

MasterOdin commented 2 years ago

PR adds a return signature of void to the setBasePort and setHighestPort functions added in #115. Without these, TS will implicitly type them as a any, which then allows someone to do anything with the "return value" in TS, losing some of the protections TS gives.

Sorry @eriktrom that I didn't catch this while I put together #141 and was looking at this function and so could have gone in 1.0.30 release.

MasterOdin commented 2 years ago

Note, without this change, it's not yet possible to replace the shim firebase-cli uses that #115 was aiming to free them of:

node_modules/portfinder/lib/portfinder.d.ts:39:17 - error TS7010: 'setBasePort', which lacks return-type annotation, implicitly has an 'any' return type.

39 export function setBasePort(port: number);
                   ~~~~~~~~~~~

node_modules/portfinder/lib/portfinder.d.ts:49:17 - error TS7010: 'setHighestPort', which lacks return-type annotation, implicitly has an 'any' return type.

49 export function setHighestPort(port: number);
                   ~~~~~~~~~~~~~~

Found 2 errors.

This may be a problem with other downstream consumers who have noImplicitAny enabled.

eriktrom commented 2 years ago

@MasterOdin - no worries - thanks for the quick work - having coffee with my partner still :)

so for the record - i don't like that we didn't catch this twice - merging - how can we catch this? - i also need to look up what the failure is behind the scenes - been in rust/c++ for a few node releases, was planning on reading release docs for node today to be honest, so any tips or google keywords here i might read up on, appreciated.

and again, thanks for the quick fix for now !!

eriktrom commented 2 years ago

Note, without this change, it's not yet possible to replace the shim firebase-cli uses that https://github.com/http-party/node-portfinder/pull/115 was aiming to free them of:

thank you

this is the kind of stability i hope to always have portfinder exhibit - it's a lot sometimes, but worth it, thankyou

do you want to update the firebase-cli ticket?

MasterOdin commented 2 years ago

do you want to update the firebase-cli ticket?

At this point, I'll just make a PR that fixes the shim :shrug:

MasterOdin commented 2 years ago

so for the record - i don't like that we didn't catch this twice - merging - how can we catch this? - i also need to look up what the failure is behind the scenes

So this failure is only for users who use noImplicitAny flag in tsconfig, as well as have not enabled skipLibCheck (new TS projects will have this added as enabled by default, but older projects that haven't specified the flag / missing it will have it off). This is less critical than say #138 as the definition file is valid without having the return signature though.

However, for both of these PRs, the answer is figuring out how to internalize what went wrong, and how can improve the CI process going forward, which I'd like to think was accomplished by first #141 and then #144 in this particular case.

eriktrom commented 2 years ago

However, for both of these PRs, the answer is figuring out how to internalize what went wrong, and how can improve the CI process going forward, which I'd like to think was accomplished by first https://github.com/http-party/node-portfinder/pull/141 and then https://github.com/http-party/node-portfinder/pull/144 in this particular case.

Part of it, honestly, is it wouldn't hurt to have someone else familiar with this code, that way when say a war starts, crypto crashes and other countries are hacking our's like crazy, while a maintainer may be trying to save a crypto company somewhere else in their headspace...

If you have any interest in becoming a maintainer, I was very impressed with your work today. Besides the new tooling and stuff that I am a bit rusty on in JS, your one of the few people who I think actually grasps this lib.

Open offer, will need to validate and talk to you in person, etc, but wanted to extend that initial open invitation.

Thanks again @MasterOdin

MasterOdin commented 2 years ago

@eriktrom I'd be happy to help, though not sure I can guarantee my availability given outbreak of a major war or something 😄

I suppose given an offer, just to lay out what my vision of my role as a maintainer would be, is that this library is mostly "done", but that I'd champion a v2 release that:

And then perhaps minor point would be to drop the usage of vows for the test suite for something like mocha or jest that has broader community usage and so would be easier for a newcomer to add to.

Anyway, my email address is in my profile if you'd like to contact me there.