Skip to content

Replace url parse by url class #1147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 6, 2020
Merged

Replace url parse by url class #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions examples/wss/client_with_proxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

var mqtt = require('mqtt')
var HttpsProxyAgent = require('https-proxy-agent')
var url = require('url')
/*
host: host of the endpoint you want to connect e.g. my.mqqt.host.com
path: path to you endpoint e.g. '/foo/bar/mqtt'
Expand All @@ -13,8 +12,8 @@ proxy: your proxy e.g. proxy.foo.bar.com
port: http proxy port e.g. 8080
*/
var proxy = process.env.http_proxy || 'http://<proxy>:<port>'
var parsed = url.parse(endpoint)
var proxyOpts = url.parse(proxy)
var parsed = new URL(endpoint)
var proxyOpts = new URL(proxy)
// true for wss
proxyOpts.secureEndpoint = parsed.protocol ? parsed.protocol === 'wss:' : true
var agent = new HttpsProxyAgent(proxyOpts)
Expand Down
25 changes: 16 additions & 9 deletions lib/connect/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

var MqttClient = require('../client')
var Store = require('../store')
var url = require('url')
var xtend = require('xtend')
var debug = require('debug')('mqttjs')

var protocols = {}
Expand Down Expand Up @@ -60,23 +58,32 @@ function connect (brokerUrl, opts) {
opts = opts || {}

if (brokerUrl) {
var parsed = url.parse(brokerUrl, true)
if (parsed.port != null) {
parsed.port = Number(parsed.port)
}

opts = xtend(parsed, opts)

if (opts.protocol === null) {
throw new Error('Missing protocol')
}
var parsed = new URL(brokerUrl)
// the URL object is a bit special, so copy individual
// items to the opts object
opts.hostname = parsed.hostname
opts.host = parsed.host
opts.protocol = parsed.protocol
opts.port = Number(parsed.port) || null
opts.username = parsed.username
opts.password = parsed.password
opts.searchParams = parsed.searchParams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure this is all the info we could possibly need from the URL? I'm worried something might be missed without just the extend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure we don't need more. And the tests agreed with me on that ;-)

This is what url.parse returns:

> url.parse("https://user:[email protected]:8080/?a=b")
Url {
  protocol: 'https:',
  slashes: true,
  auth: 'user:password',
  host: 'www.example.com:8080',
  port: '8080',
  hostname: 'www.example.com',
  hash: null,
  search: '?a=b',
  query: 'a=b',
  pathname: '/',
  path: '/?a=b',
  href: 'https://user:[email protected]:8080/?a=b'

Kind regards,
Hans

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: @nosovk already approved on September 2 so its not just me ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should actually be opts.host = parsed.hostname
This project uses host as the name for hostname (just domain, without port)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the 11 locations where opts.hostname is being used ;-)
https://github.com/mqttjs/MQTT.js/search?q=opts.hostname

and currently (without my PR) the results from url.parse are copied into opts.

So currently:

The comments in type definitions seem to be a bit ahead of the code ;-)

But anyway:

For me its just a hobby, and I saw the issue and thought I'd help out.
If you all feel it can be done way better than I did, feel free!
(its only a few lines of code and from my PR you know where to look ;-))
That would save me the effort of rebasing (again) as well.

If you still want me to rebase because you are all way too busy, just let me know when its safe to do so I don't need to rebase a third time ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appreciate your contributions just doing due diligence :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in that case: #1148 is also still open ;-)
It would convenient for me if that could be either merged or fixed in some other way better way :-)
(e.g. generate fresh certs if not available etc )

opts.protocol = opts.protocol.replace(/:$/, '')
}

// merge in the auth options if supplied
// legacy support for url.parse objects (now deprecated in node.js)
parseAuthOptions(opts)

// support clientId passed in the query string of the url
if (opts.searchParams && typeof opts.searchParams.get('clientId') === 'string') {
opts.clientId = opts.searchParams.get('clientId')
}

// legacy support for url.parse objects (now deprecated in node.js)
if (opts.query && typeof opts.query.clientId === 'string') {
opts.clientId = opts.query.clientId
}
Expand Down
4 changes: 2 additions & 2 deletions lib/connect/ws.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const WS = require('ws')
const debug = require('debug')('mqttjs:ws')
const duplexify = require('duplexify')
const urlModule = require('url')
const Buffer = require('safe-buffer').Buffer
const Transform = require('readable-stream').Transform

let WSS_OPTIONS = [
Expand Down Expand Up @@ -69,7 +69,7 @@ function setDefaultBrowserOpts (opts) {
if (typeof (document) === 'undefined') {
throw new Error('Could not determine host. Specify host manually.')
}
const parsed = urlModule.parse(document.URL)
const parsed = new URL(document.URL)
options.hostname = parsed.hostname

if (!options.port) {
Expand Down
3 changes: 1 addition & 2 deletions test/browser/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
'use strict'

var mqtt = require('../../lib/connect')
var _URL = require('url')
var xtend = require('xtend')
var parsed = _URL.parse(document.URL)
var parsed = new URL(document.URL)
var isHttps = parsed.protocol === 'https:'
var port = parsed.port || (isHttps ? 443 : 80)
var host = parsed.hostname
Expand Down
2 changes: 1 addition & 1 deletion test/mqtt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe('mqtt', function () {
(function () {
var c = mqtt.connect('foo.bar.com')
c.end()
}).should.throw('Missing protocol')
}).should.throw('Invalid URL: foo.bar.com')
})

it('should throw an error when called with no protocol specified - with options', function () {
Expand Down