add app.listenPromise

[caub]

Proposing this new method

nodejs/node#21482

or maybe we could have a breaking change for express 5, where .listen return a Promise if no callback is given?

I think it's be great, since doing:

const server = app.listen(process.env.PORT || 3000);
console.log(server.address().port)

is unsafe (it can throw since .listen is async, and .address not necessarily available just after)

but I guess you'll want to preserve the API chaining style for .listen

maybe a app.ready() returning a Promise rather?

[wesleytodd]

Hey @caub! I think the idea of app.listen returning a promise if no callback was specified is the best idea here. I would be interested in hearing other contributors opinions, but I imagine if you put together a PR with just¹ that it would setup the conversation point.

1: I mean remove all the other changes you have in this PR, and just use native promises.

[caub]

it could be written without util.promisify in something like

  var args = Array.prototype.filter.call(arguments, function (arg){
    return typeof arg !== 'function'
  });

  return new Promise(function (resolve, reject) {
    server.listen.apply(server, args.concat(function (err) {
      if (err) return reject(err);
      resolve(server);
    }));
  });

[caub]

@wesleytodd the problem is when people use the return value of app.listen, with no callback. Making it return a Promise would break this

[wesleytodd]

This is bringing back memories now. I think this issue has already undergone some discussion but I cant remember where. I searched PR's and issues but couldn't find it.

You are absolutely right that returning a promise breaks the existing behavior, but I was thinking it could be one of the breaking changes in 5.0. The change would be to resolve with the server or reject with an error. I am not sure I like the change, but that is what I was thinking the discussion would be about.

[LinusU]

I've done something similar back in an internal framework for a previous company, and ended up reverting the change.

The problem is that listen currently fills two functions:

1. it creates an http.Server
2. it calls listen on that server

That is why the listen function currently both return a value (the Server) and takes a callback.

I think that the "best" or "most correct" or whatever way to do it would have that as two methods. First you create a server, then you call listen on that. But then the question is whether we should return a plain http.Server (which then will not support a promisified .listen) or whether to return our own representation of an http.Server.

That would also then make it "harder" or "more complicated" to just start listening on a port, since it would require two calls.

Maybe a createServer, or just server, method could be provided in addition to a revamped listen that returns a Promise<void>.

app.server().listen(3000, () => console.log('Listening'))
app.listen(3000).then(() => console.log('Listening'))

const server = app.server()

server.listen(0, () => {
  console.log(`Listening on ${server.address().port}`)
})

We could even make .listen return a Promise of server.address(), I think that would be quite neat!

app.listen(0).then(({ address, port }) => {
  console.log(`http://${address}:${port}`)
})

and also default the port to 0 (or maybe even Number(process.env.PORT || '0'), but maybe too much magic?)

app.listen().then(({ address, port }) => {
  console.log(`http://${address}:${port}`)
})

[dougwilson]

I'm going to close this since it's been 2 months and there hasn't been any update to the pr, responses to the two above collaborator comments, and the changes don't even have passing tests after all this time.

If there is desire this pr can always be reopened or a new one created.

[caub]

I tried to make it, (see my last push), so that it works like before when passing a callback, or return a Promise else

I get this only error though: and couldn't find how to solve it

 1) res .sendFile(path, fn) should invoke the callback when client aborts:
     TypeError: app.address is not a function
      at Test.serverAddress (node_modules/supertest/lib/test.js:55:18)
      at new Test (node_modules/supertest/lib/test.js:36:12)
      at Object.obj.(anonymous function) [as get] (node_modules/supertest/index.js:25:14)
      at Context.<anonymous> (test/res.sendFile.js:277:34)

[LinusU]

I would like for us to have a discussion about the points I brought up before landing this ☺️

[dougwilson]

Should a new issue be opened to discuss? I read through the above and wasn't super obvious what the discussion points were, and perhaps a more meta issue may help, especially since there are no promise anything in express today so adding this one promise thing just seems off at the surface.

The reopen button is disabled on the pr apparently so i can't reopen if i wanted to, sorry.

[caub]

@LinusU I rather think express should stay this simple wrapper of node's http.Server

I'd just like to add app.close that would call server.close in addition of this promisified listen (that might come in nodejs as well some day)

@dougwilson no problem, anyway, we should first have an idea if what to do

the problem is waiting for server.listen, doing var server = app.listen(); console.log(server.address().port) in a synchronous way sometimes fail

here's my changes, since they are no longer updated

------------------------------ lib/application.js ------------------------------
index 91f77d24..6eb797f9 100644
@@ -615,7 +615,17 @@ app.render = function render(name, options, callback) {
 
 app.listen = function listen() {
   var server = http.createServer(this);
-  return server.listen.apply(server, arguments);
+  var hasCb = typeof arguments[Math.min(1, arguments.length-1)] === 'function';
+  // if a callback is given, use it
+  if (hasCb) return server.listen.apply(server, arguments);
+  // else return a Promise
+  var args = Array.prototype.slice.call(arguments);
+  return new Promise(function(res, rej) {
+    server.listen.apply(server, args.concat(function(err) {
+      if (err) return rej(err);
+      res(server);
+    }));
+  });
 };
 
 /**

----------------------------- test/res.sendFile.js -----------------------------
index ccec3f40..305b3758 100644
@@ -112,9 +112,10 @@ describe('res', function(){
         cb();
       });
 
-      var server = app.listen()
-      var test = request(server).get('/')
-      test.expect(200, cb);
+      app.listen().then(function(server) {
+        var test = request(server).get('/')
+        test.expect(200, cb)
+      })
     })

[dougwilson]

press should stay this simple wrapper of node's http.Server

I'm not sure what this mean. Express does not wrap http server at all. That's why there is no app.close etc. An express app can be multiple servers / ports. This is how you can make an express app be both http and https at the same time, for example.

The overall issue is that our .listen is just the Node.js .listen. if node.js makes it return a promise than it will here too, and I think that is better for the ecosystem anyway, as I'm sure if having .listen return a promise is useful, then it is useful being in core and not confined to express only.

[caub]

@dougwilson ok, makes sense, thanks

here's the nodejs issue I opened at that time: nodejs/node#21482, will try to expand discussion there so

[LinusU]

Basically, I just don't want us to stop returning the http.Server instance, even if there is a callback provided, this is valid code that I've seen in the wild:

const server = app.listen(0, () => {
  console.log(`Listening on ${server.address().port}`)
})

which would break by that.

I'm not sure that promisifying listen is the right approach since it's not actually callback that is passed in, it's an event listener. In http.Server#listen, the function passed won't receive an error if the server fails to listen, and it will be called multiple times if the server emits the listening event more than once.

[dougwilson]

Right, it is not a callback function. Node.js core has several places where this happens: they take a function as the last arg and just attach that as an event listener and that's it. It is confusing to users because it looks and generally acts like a callback but yet is not one. Usually only callbacks are promisified.