Improvements for getApi() #253
Conversation
…nce of NgrokClient
There was a problem hiding this comment.
I'm not so sure of the getProcessUrl method here. It seems like it may not work everywhere, and even if it does it will be a breaking change.
If you need to check whether the process is running, we could look into something like tcp-port-used.
But in reality, I'm not sure exactly how useful checking the port is in use would be beforehand. You have to handle whether you get a client back or not, so would it be worse to handle the issue when you try to fetch the client or when you make the API call? Especially since the API could be closed down later and you wouldn't know about it.
I would suggest that perhaps the getApi method stays the same, with the intended use that it is called in a program where ngrok.connect is also called. Then we expose the NgrokClient object from the main index so you can create your own. And finally, we need to extend the error handling of NgrokClient so that if it tries to make a call and receives an error with the code ECONNREFUSED then it raises a different error (an NgrokApiMissing or NgrokConnectionRefused error, something like that) so that it can be handled by applications trying to use the API when they can't know if it is running.
What do you think?
|
|
||
| const child = platform === 'win32' | ||
| ? spawn(`(Get-NetTCPConnection -LocalPort ${port}).LocalAddress`, { shell: 'powershell.exe' }) | ||
| : spawn('sh', ['-c', `lsof -n -i :${port} | grep LISTEN`]) |
There was a problem hiding this comment.
This seems like it could be fairly brittle? It requires PowerShell to be installed, which I don't think is the default for Windows.
There was a problem hiding this comment.
The initial research that led me down this route turned up that PowerShell is installed by default for all versions of Windows after Windows 7 and Server 2008, both of which are officially unsupported at this point. And this was simpler to implement with PowerShell than CMD.
However, I recognize your points above. Happy to scrap this and focus more on simply exporting NgrokClient, and extending its error handling. More questions to come about that.
| @@ -39,7 +39,7 @@ declare module "ngrok" { | |||
| /** | |||
| * Gets the ngrok client API. | |||
| */ | |||
| export function getApi(): NgrokClient | null; | |||
| export function getApi(): Promise<NgrokClient | null>; | |||
There was a problem hiding this comment.
This would be a breaking change to the public API.
There was a problem hiding this comment.
Ack. Going to dial back on this as mentioned above.
|
Closing this old PR out and will submit a fresh one with the feedback received |
Solution for #252
Changes
index.js-getApi()ngrokClientis defined, return if so. Else, CallgetProcessUrlto get the process URL for port 4040, and create a new instance ofNgrokClient. Fallback to null.src/process.jsgetProcessUrlwhich spawns a child process and makes the platform appropriate query for port 4040's process URL.ngrok.d.tsgetApi()type definition to return a nullable promise.test/ngrok.guest.spec.js"getting internal api wrapper"test case to callgetApi()with async/await."should be an instance of NgrokClient".Tests run successfully on Mac, Windows 10, and Linux. Open to any improvements!