search

tagomoris / presto-client-node

Distributed query engine Presto client library for node.js
MIT License
125 stars 57 forks source link

Support default port for nextUri #37

Closed clarkmalmgren closed 4 years ago

clarkmalmgren commented 4 years ago

When the nextUri doesn't contain an explicit port, href.port is an empty string. Ultimately, it will fail in node's underlying check to ensure that it is a valid port.

This PR adds support for default ports when the nextUri is something like this:

https://presto.mycompany.com/v1/statement/queued/20200722_041411_00666_zd1yb/yca3b19384759874508288e6afc89f627d59d1c1/2

It uses the default port of 443 for https: and assumes http: and thus port 80 for anything else.

tagomoris commented 4 years ago

The value type of new URL("url").port is always string. Is it intentional to assign numbers?

Screen Shot 2020-07-22 at 17 18 59
clarkmalmgren commented 4 years ago

Either way will work. When debugging I dove really far down into the underlying node implementation and it will coerce a string to a number. Let me know if you'd prefer that I keep them strings for consistency.

tagomoris commented 4 years ago

I prefer consistency always :D Or a comment like "href.port is a String but Integer works well too" would be great.

clarkmalmgren commented 4 years ago

@tagomoris, I updated to strings for consistency. Sorry for the delay. Let me know if you need me to do anything else.

tagomoris commented 4 years ago

Thank you! Published as v0.8.1. https://www.npmjs.com/package/presto-client/v/0.8.1