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
882 stars 95 forks source link

Webpack doesn't like portfinder #113

Closed PeterWone closed 3 years ago

PeterWone commented 3 years ago

Webpack makes my head hurt. I have many imports, portfinder alone upsets it. As you can see from the following, webpack seems to have decided to build portfinder from source.

[webpack-cli] Compilation finished
assets by status 810 KiB [cached] 1 asset
orphan modules 126 bytes [orphan] 3 modules
runtime modules 668 bytes 3 modules
modules by path ./node_modules/highlight.js/lib/languages/*.js 1.01 MiB 185 modules
modules by path ./node_modules/portfinder/ 29.4 KiB
  ./node_modules/portfinder/lib/portfinder.js 14.4 KiB [built] [code generated]
  ./node_modules/portfinder/node_modules/debug/src/index.js 263 bytes [built] [code generated]
  ./node_modules/portfinder/node_modules/debug/src/browser.js 4.62 KiB [built] [code generated]
  ./node_modules/portfinder/node_modules/debug/src/node.js 5.87 KiB [built] [code generated]
  ./node_modules/portfinder/node_modules/debug/src/debug.js 4.29 KiB [built] [code generated]
modules by path ./node_modules/highlight.js/lib/*.js 37.8 KiB
  ./node_modules/highlight.js/lib/index.js 11.5 KiB [built] [code generated]
  ./node_modules/highlight.js/lib/highlight.js 26.3 KiB [built] [code generated]
10 modules

ERROR in D:\_vscode-printing-free\vsc-print\src\extension.ts
[tsl] ERROR in D:\_vscode-printing-free\vsc-print\src\extension.ts(7,29)
      TS2307: Cannot find module 'portfinder'.

There are other issues. Consuming portfinder.d.ts like this

import * as portfinder from "portfinder";

results in Typescript complaining that basePort is readonly. There's a simple oh-shut-up like this

(portfinder as any).basePort = 52000;

but, well, blech. I know this is a separate issue, but I really think the best answer is to re-factor this library using Typescript classes and let the compiler produce the artefacts for both languages. I mention this here because I strongly suspect this would make the webpack silliness go away.

eriktrom commented 3 years ago

As you can see from the following, webpack seems to have decided to build portfinder from source

What version of webpack, v4 or v5?

Regardless, webpack should NOT compile portfinder as portfinder is a node only module, it does not have an entrypoint for the browser, it should not be chunked/transpiled/etc - if using typescript, use ts-node instead for this in CLI/binary applications.

but I really think the best answer is to re-factor this library using Typescript classes and let the compiler produce the artefacts for both languages There are other issues. Consuming portfinder.d.ts like this

I will answer these q's as one: (if ur confused, please comment, i'm summarizing heavily here)

B/c this is only a .d.ts file, and interfaces are not actually imported in typescript(classes are but..), they are only consulted at build time by typescript, changing the entire library to typescript is not necessary. I would accept a PR though for changing the portfinder.d.ts to allow that property to be writable though, which is all that is needed. You should just pass in the start port though instead(the signature allows passing a host and port, the port will be the port it starts iterating from). That said, there are large OSS projects whom have set the exports.basePort, so that will remain an option indefinitely, and thus we should change that property to writeable, pr welcome.


but I really think the best answer is to re-factor this library using Typescript classes and let the compiler produce the artifacts for both languages

more on this, but ancillary - the artifact produced - would not be a webpack chunk - which is likely all I would have responded with had I not literally been asked this same question in an different context twice this week already (making explaining it here, easier, so I did so, I did my best that is, webpack is not explainable, even with infinite time, jk)


Feel free to ask any questions that may remain. Use ts-node though (https://github.com/TypeStrong/ts-node#how-it-works) and skip webpack when invoking portfinder to find an open port.

eriktrom commented 3 years ago

closing, b/c I hope my explanation suffices - there is no fix to be had - open a pr if u change typescript interface file

PeterWone commented 3 years ago

webpack is not explainable, even with infinite time

...because it's a moving target solving a problem that is properly the domain of the browser (reducing the overhead of lots of small files)

TS import has decided that portFinder is a const because it's declared at the root. The only way to fix this is to express it as a property of a class, and export that. The members of internals become private members of the class, and all the functions become methods. But it's such a PITA, and then all the tests have to be rewritten...

eriktrom commented 3 years ago

import * as portfinder from portfinder not working your saying ...

import module from 'module' const portfinder = module.require('porfinder');

does that work?

eriktrom commented 3 years ago

(u may need to read the node docs to complete that thought, fyi, i have had success with that idea though in ts.

PeterWone commented 3 years ago

No, that's how I was doing it before I tried to add webpack.

Webpack had a hissy fit until I used import * as portfinder from "portfinder"; and then I had the compile from source nonsense.

eriktrom commented 3 years ago

ROFL

eriktrom commented 3 years ago

drop a note when if u have success OR in a couple days with still no success - i think more deeply in meantime

PeterWone commented 3 years ago

I want a friggin teeshirt that says webpack is not explainable, even with infinite time

eriktrom commented 3 years ago

I wear a large, save me one :)

PeterWone commented 3 years ago

Much as I like your library I really need to hack something together in TS so I can ship. To that end I need to hack out just the bits I need and implement as a TS class. I would be grateful for some sanity checks and guidance -- you know what you meant by your code, I'm only guessing.

If I've understood your code, the gist of it is

It's obvious why you remove the listener and close the server when you find an available port but I'm not clear on why onerror removes the listener or how it is replaced for the next try.

I'll be using Promise<number> as the return value of getPort so I'll use a closure to bring resolve and reject into scope for onListen and onError rather than an explicit callback.

eriktrom commented 3 years ago

Your summary looks about correct. Here are some additional notes, and hopefully answers to ur q's.

how it is replaced for the next try.

Portfinder is hands off about this.

It is not replaced for the next try in this module. Write some code that loops and tries again for u, on perhaps the next port. Currently, I know of many packages that use portfinder, they are all cli apps - ember-cli, webpack-cli, ng-cli, circle ci(not sure how, but i broke it and boy was that a bad move). Point is, when the port is not open, they either exit and report that information, or they could try the next port, although none i know of do atm.

Internally, this module's real job is to test that a provided port is not in use on any interface available to the operating system - this is key. 0.0.0.0 vs 127.0.0.1 vs ::1 - are checked - in fact, all internal and externally facing interfaces, both ipv6 and ipv4 for those interfaces are checked to see if they are using the port u give, skipping interfaces that are sudo locked or not usable (like bluetooth or the mac touchbar) by simply skipping those port ranges (above 40000, below 1024). It also handles the mixed bag of syscall errors thrown by different os' when trying to use a port that is used by another interface.

I'm not clear on why onerror removes the listener

onerror is needed to catch errors thrown from interfaces that fail to open a socket on that port, allowing us to early abort looking for said port on the rest of the interfaces. If we didn't close and then exit in such a circumstance, we would iterate to the next interface, which would say yes the port is open for business, when the another interface has in fact taken it.


If u do try to implement this from scratch (and reminder to self):

One important note: u can open a port on an interface, without error, but that does not mean u can actually use the interface you opened the socket on, and upon doing so, you may get an error. A long term fix would be to pipe some text through the socket and if echo'ing back that text does not work, consider the port + interface in use, exit. This would remove a lot of the hacky error handling, but it's also a dangerous change, and there are too many consuming libraries using this lib for ci on github and for cli tools for client side js apps, to make that change without bumping major semver - which would then require i go bump all consumers package.json, lest they never bump the major version and thus don't get the change anyway. So I haven't. Actually, I did once, for 5 hours. Then got on a plane. Had hundreds of emails when I landed. Reverted. 🤷‍♀️

Last note, if u try to test a port below 1024, that u know is open, don't forget to use sudo :)

eriktrom commented 3 years ago

ps - i have not read the codebase for a couple years - i just did - one things sticks out - nextPort is only used by getPorts - which is not used very often but does have good test coverage.

it conflates the onError callback though, b/c what I said about closing the socket is true, although with getPorts, its true, but we then do iterate to the next port depending on how many ports in a row u wanted to check an open range for

clarification, im glad i checked

PeterWone commented 3 years ago

Thanks, Erik. I should be able to bang something up to handle my narrow use case.