refactor: parse default http ports in url, fixes #59 #61
Conversation
This change correctly parses default http ports in url for rest and grpc packages.
| @@ -47,7 +47,9 @@ export class QdrantClient { | |||
| } | |||
| const parsedUrl = new URL(url); | |||
| this._host = parsedUrl.hostname; | |||
| this._port = parsedUrl.port ? Number(parsedUrl.port) : port; | |||
| const getPort = (url: string): number | null => | |||
| url.includes(':443') ? 443 : url.includes(':80') ? 80 : port; | |||
There was a problem hiding this comment.
what if url has localhost:8000? url.includes(':80') will still match
There was a problem hiding this comment.
In that case parsedUrl.port on line 52 will return 8000, so getPort will not be invoked.
There was a problem hiding this comment.
ok, in which case getPort will be invoked? If port is not specified explicitly? If so, why are we doing those checks for url.includes?
There was a problem hiding this comment.
Invoked in case of urls having default ports (443 for https and 80 for http) or no port specified.
#61 (comment)
|
Thanks for looking @generall , @kartik-gupta-ij For context, the url parser This change fixes this behavior. The issue was seen here - #59 |
This change correctly parses default http ports in url for rest and grpc packages.