Consider having Application.listen() return a promise that resolves when the server has started

[jrop]

Currently, the signature of node's Server.listen is:

Server.listen(port, callback) // plus a few more overloads...

The given callback is attached to the Server 'listen' event, that gets emitted once the socket is listening for incoming messages. What would the implications be of Koa's Application.listen returning a promise, in the spirit of all the async/await changes coming down for v2.x?

[PlasmaPower]

That would break passing app to supertest, or anything else that calls .listen. I'd be fine with returning a promise, but I'd keep the callback as long as node does.

[PlasmaPower]

Also, we are currently returning the server from .listen, so returning a promise would be a breaking change either way.

[jrop]

@PlasmaPower It can still take a callback as an argument, and return a promise at the same time; consider:

  listen() {
    debug('listen');
    const server = http.createServer(this.callback());
    const args = Array.prototype.slice.call(arguments)

    return new Promise(function (resolve, reject) {
      const callback = args[args.length - 1]
      function onListening(err) {
        if (typeof callback == 'function')
          callback(err)
        return err ? reject(err) : resolve(server)
      }

      if (typeof callback == 'function')
        args[args.length - 1] = onListening
      else
        args.push(onListening)
      server.listen.apply(server, args)
    })
  }

[jrop]

As for returning the server, my code above resolves to the server object, but yes, it is still a breaking change.

[PlasmaPower]

@jrop Yes I know I was suggesting doing exactly what you said. I'll make a PR for it in a bit.

[jrop]

Awesome, I thought you might be; if you want I can create the PR, otherwise I'm fine with it either way.

[PlasmaPower]

It looks like this would break pretty much all our tests. I'd prefer to wait until node changes their .listen to change ours. Edit: or maybe we could do something about this when/if we switch the request library.

[fl0w]

@PlasmaPower you already made notice of this; but this change would be ultra-major IMO. It somewhat ties into #703 as well. This might be something to consider once there's a consensus regarding tests and a potential rewrite?

[jonathanong]

this would require wrapping the http server, which isn't really worth it. if https://github.com/normalize/mz wrapped it, then maybe, but it shouldn't be done in koa.

[PlasmaPower]

@jonathanong It wouldn't really require to much extra code, if we use event-to-promise we could simply do return eventToPromise(server, 'listened').

[fl0w]

Maybe this one can be closed as wontfix?

[jonathanong]

@fl0w agreed!

[Ajaxy]

@PlasmaPower eventToPromise(server, 'listened') did not work form me. It seems that that event is not documented anywhere.