moscajs / mosca

MQTT broker as a module
mosca.io
3.2k stars 508 forks source link

mqtt over websocket on different path #606

Closed namgk closed 7 years ago

namgk commented 7 years ago

Fixes this #605

namgk commented 7 years ago

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

namgk commented 7 years ago

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

mcollina commented 7 years ago

if no path is specified in attachHttpServer, and a mqtt client connect to 'ws:///', should the server ignore the 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 commented 7 years ago

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 commented 7 years ago

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

namgk commented 7 years ago

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

namgk commented 7 years ago

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 commented 7 years ago

Yes, 'interfaces', not 'interface'.

namgk commented 7 years ago

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 commented 7 years ago

Would you mind sending a fix?

namgk commented 7 years ago

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

namgk commented 7 years ago

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

namgk commented 7 years ago

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

mcollina commented 7 years ago

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

namgk commented 7 years ago

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 commented 7 years ago

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