Replace url-parse lib with native URL interface #678
Conversation
| }; | ||
|
|
||
| fields = openFrames(url.vhost, config, sockopts.credentials || credentials.plain(user, pass), extraClientProperties); | ||
| fields = openFrames(url.vhost, url.searchParams, sockopts.credentials || credentials.plain(user, pass), extraClientProperties); |
There was a problem hiding this comment.
CAUTION (maybe) vhost used here is not a part of a URL instance.
Then again it wasn't a part of the legacy url.parse / url-parse result either so this was undefined anyway. The only place I found where it wasn't undefined was in the bad URL test.
There was a problem hiding this comment.
this sounds like a bug. can we fix it? vhost is important
or you mean that there is no way to include vhost in url?
amqp://${config.username}:${config.password}@${config.hostname}:${config.port}/${config.vhost} is a commonly used structure for amqp urls, and, to my knowledge, currently it works.
|
Thanks @jonaswalden. 7acbfd7 details why url-parse was introduced in the first place. I suspect that now amqplib only supports node 10 and up, the standard url parser will be fine. Unless this isn't so, I'm in favour of this PR - the fewer 3rd party libs we import the better |
|
@jonaswalden Could you please rebase this? |
|
@cressie176 wow, ok. good to know! seems like that's covered https://github.com/BonnierNews/amqplib/blob/main/test/connect.js#L54 |
|
hold on, I screwed something up |
solved. @kibertoad good to go? |
| @@ -104,37 +103,30 @@ function connect(url, socketOptions, openCallback) { | |||
|
|
|||
| var protocol, fields; | |||
| if (typeof url === 'object') { | |||
| protocol = (url.protocol || 'amqp') + ':'; | |||
| protocol = url.protocol || 'amqp:'; | |||
There was a problem hiding this comment.
protocol no longer gets : appended to it, is it intentional?
| @@ -29,27 +29,27 @@ suite("Credentials", function() { | |||
| } | |||
|
|
|||
| test("no creds", function(done) { | |||
| var parts = urlparse('amqp://localhost'); | |||
| var parts = new URL('amqp://localhost'); | |||
| var creds = credentialsFromUrl(parts); | |||
| checkCreds(creds, 'guest', 'guest', done); | |||
| }); | |||
| test("usual user:pass", function(done) { | |||
There was a problem hiding this comment.
can we add test case for the vhost, using the
amqp://${config.username}:${config.password}@${config.hostname}:${config.port}/${config.vhost} structure?
|
@jonaswalden Do you need any help with finishing this? |
Engine specified to node >= 10.
Should be no need for a lib to polyfill the
URLinterface or implement the legacyurl.parsein a browser environment.