Closed MasterOdin closed 2 years ago
thanks @MasterOdin - i owe you - for organizing the pr's (like #140 as a no no :)) and i even made a comment on #139 (here: https://github.com/http-party/node-portfinder/pull/139#discussion_r945157882) - only to find out that you had addressed that comment here, in a super simple way.
again, thanks a bunch.
it's people like you that have made maintaining this repo over the past 8 years or so worth it. :)
Yeah, I do think #140 is the superior solution if you were willing to maintain a typescript library, but this is a definite "nice to have" if rewriting to TS is out of the question (which is totally fair). I suppose an additional step would be to have a test TS file that gets executed as well (similar to how DefinitelyTyped works), but forward progress.
I forgot to put into the PR that the reason I didn't add typescript as a dev dependency is mostly in that you'd have to go back to a very ancient version of TS to support Node 0.12 (~2.1 or something?). If the library drops node < 4, then could add this to the devDependencies.
it's people like you that have made maintaining this repo over the past 8 years or so worth it. :)
Happy to help 😄
This PR modifies the current CI so that it runs the typescript compiler over the
portfinder.d.ts
file to minimally check that it's valid TS. A potential further step would be to use something like tsd to write unit tests on the types, but the types here are relatively simple that I'm not sure how much benefit that'd bring.This PR will currently fail the new CI step (test run) until #139 is merged. Once #139 is merged, I'll rebase
master
onto this branch and it should then now pass.Also, this PR only makes sense if #140 were to be rejected.