Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

mqtt over websocket on different path #606

Merged
merged 5 commits into from Mar 14, 2017
Merged

Conversation

namgk
Copy link
Contributor

@namgk namgk commented Feb 13, 2017

Fixes this #605

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you add a unit test for this?

@namgk
Copy link
Contributor Author

namgk commented Feb 13, 2017

I don't see we have any test for attachHttpServer yet, I'm creating a new one then.

@namgk
Copy link
Contributor Author

namgk commented Feb 16, 2017

added some unit tests, however, there are a few things:

  • if no path is specified in attachHttpServer, and a mqtt client connect to 'ws://<host>/<any_path>', should the server ignore the <any_path> part and still serve such client as if it was connecting to just 'ws://<host>'?
  • was it your intention to always put a mqtt server on port 1883 as default (the case of empty option provided to mosca server)? This doesn't make much sense in the case where you are attaching to an existing http server.

"uglify-js": "^2.4.16",
"underscore": "^1.7.0"
"underscore": "^1.7.0",
"ws": "^1.0.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be a real dependency, not a devDependency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

forget about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what do you mean? Would it be ok now then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's ok, my bad.

@mcollina
Copy link
Collaborator

if no path is specified in attachHttpServer, and a mqtt client connect to 'ws:///<any_path>', should the server ignore the <any_path> part and still serve such client as if it was connecting to just 'ws://'?

Keep the same behaviour as it was before, like it's now in code.

was it your intention to always put a mqtt server on port 1883 as default (the case of empty option provided to mosca server)? This doesn't make much sense in the case where you are attaching to an existing http server.

Check out https://github.com/mcollina/mosca/blob/master/examples/Server_With_All_Interfaces-Settings.js#L16-L19.

@namgk
Copy link
Contributor Author

namgk commented Feb 16, 2017

I know it is possible to have server with all interface.

My point is that when I'm attaching to an existing http server, I don't want the port 1883 to be occupied, that's the reason why I'm using mqtt-over-ws.

@mcollina
Copy link
Collaborator

You can just specify the interfaces you want to listen to, or none.

@namgk
Copy link
Contributor Author

namgk commented Feb 16, 2017

the problem is if I specify none, there will still be a mqtt server listens on port 1883.

@namgk
Copy link
Contributor Author

namgk commented Feb 16, 2017

oh do you mean I should do

var mqttServer = mosca.Server({interface:[]})

?
What I'm using now is

var mqttServer = mosca.Server({})

And this occupies my 1883 port.

@mcollina
Copy link
Collaborator

Yes, 'interfaces', not 'interface'.

@namgk
Copy link
Contributor Author

namgk commented Feb 18, 2017

The following test failed, it's throwing Error: no interface defined

it.only("should not occupy 1883 port while attached to http server", function(done) {
    server = http.createServer();
    mqttServ = new mosca.Server({interfaces:[]});
    mqttServ.attachHttpServer(server);
  });

@mcollina
Copy link
Collaborator

Would you mind sending a fix?

@namgk
Copy link
Contributor Author

namgk commented Feb 18, 2017

sure, I just want to make sure your intention in that case. I'll send another commit.

@namgk
Copy link
Contributor Author

namgk commented Feb 26, 2017

fixed empty interfaces options, refactored the tests to reflect this.

@namgk
Copy link
Contributor Author

namgk commented Mar 6, 2017

hi Matteo, do you have a plan to merge? Thanks

@mcollina
Copy link
Collaborator

mcollina commented Mar 7, 2017

I'm traveling till the end of this week, I will have a look afterwards.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work, just a minor nit.

lib/options.js Outdated
// if (opts.interfaces.length === 0) {
// result.addError('no interfaces were defined');
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this has been commented? If this is not needed anymore, we should just remove it.

@namgk
Copy link
Contributor Author

namgk commented Mar 13, 2017

Hi Matteo, that code block was under the assumption that there should be minimum one interface in the option. However this doesn't hold anymore due to the case where you are attaching the mqtt stack on an existing http server.

I deleted that code block.

@mcollina
Copy link
Collaborator

perfect, I'll try to assemble a release asap.

@mcollina mcollina merged commit f1c814a into moscajs:master Mar 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants