diversario / node-ssdp

node.js SSDP client/server.
MIT License
274 stars 118 forks source link

Server.start needs to return promise or accept a callback #70

Closed andrewshatnyy closed 7 years ago

andrewshatnyy commented 7 years ago

There is no way to say when servers stars listening. Which makes tests depended on timeouts.

// when server.start takes a callback
describe('Client Server', (done) => {
  const server = new Server(options);
  server.start(done);
});

or with promise (preferred) which is yieldable

// when server.start returns a `new Promise()`
describe('Client Server', function* cs() {
  const server = new Server(options);
  yield server.start();
});

Please let me know if anyone needs this and which is more useful. I'd patch the server gladly.

diversario commented 7 years ago

@andrewshatnyy Server#start does take a callback that's passed to https://github.com/diversario/node-ssdp/blob/master/lib/index.js#L194, which calls it after every socket has been bound.

andrewshatnyy commented 7 years ago

Not sure if I am missing something but I if use SSDP server https://github.com/diversario/node-ssdp/blob/master/lib/server.js#L46 I have no callbacks.

I also see no callbacks in tests. I ended up binding to 'listen' event on the first '0.0.0.0' socket to tell when the socket is ready in tests.

like so:

    return new Promise((success, failure) => {
      if (!server.sock) failure();
      server.sock.once('listening', success);
      server.sock.once('error', failure);
    });

Should I call server._start(done) then?

diversario commented 7 years ago

Oh, I see what you're saying. This isn't right. Of course, Server#start should be handling a callback properly.

I do like the promise suggestion but at this time I would like to focus in having a consistent callback interface.

diversario commented 7 years ago

Fixed in v3.1.0