Polka: Remove Promise around Listen

[lukeed]

This seemed like a good initially just because of then/catch, but there are a few problems with this:

1. Calling .listen() does not return the Polka instance directly, so it forces listen to always be called last.

2. It also prevents one-line initializers for a server.

   const server = polka().listen(3000);
   //=> server is Promise, not Polka instance
   

3. The top-level then and catch blocks make it seem like catch is the global catch-all handler.

Instead, .listen() would expose the underlying server.listen method directly, meaning that signature is the same as all other servers.

polka().listen(PORT, err => {
  if (err) throw err; // or whatever you want to do
  console.log(`> Ready on localhost:${PORT}`);
});

This would also remove 5 lines of code 😆

[Youngestdev]

I think the new development is cool !

[lukeed]

Sorry @Youngestdev, which do you mean?

[Youngestdev]

polka().listen(PORT, err => {
  if (err) throw err; // or whatever you want to do
  console.log(`> Ready on localhost:${PORT}`);
});

That looks cool

[lukeed]

Got it, thanks!

[yamalight]

@lukeed my 2c - I like the promise version more (since I've been using async/await _a lot_).
I think just clarifying what .listen() returns in docs would be sufficient

[lukeed]

Thanks @yamalight~! I'm still slightly in favor of removing the Promise version because I'm seeing a lot of early users trying to server = polka().listen(PORT)... especially for tests. The "drawback" is needing to separate this into two lines, which isn't the end of the world but it slightly annoying since no one's used to it. 🤔

[yamalight]

@lukeed how about allowing to do server = await polka().listen(PORT)?

[lukeed]

That's far more complicated and would usePromise on the main application, affecting performance.

[yamalight]

fair point 😃

[terkelg]

I think we should remove the promise. I actually had some problems with. It took me a while to realise that .listen() not returns an http.Server object.

This is also another step towards better express compatibility like #29 ... and less code! What's not to like.

[r0mflip]

Changing to something like

listen(...args) {
  (this.server = this.server || http.createServer()).on('request', this.handler);
  this.server.listen(...args);
  return this.server;
}

would be closer to honouring options for net.Server

[lukeed]

Yup! That's actually exactly the code I was going to write. I just (still) haven't decided if this should happen or not. Pros & cons for each side.

[r0mflip]

Yeah. And I think promisification can be done by

// sketchy, untested code ahead

const {promisify} = require('util');
const app = require('polka')();

app.use((req, res) => {
  res.end(200, 'OK');
});

const start = promisify(app.server.listen);  // assuming server already exists

start()
  .then(...)
  .catch(err => ...);

(😜)

[peappeap]

now i was start node index.js and i get ---- attempt to reconnect has failed--- in browser how can i do sir ?

[mrkishi]

How about exposing a promise as a server property?

const server = polka().listen(PORT)
server.started.then(f).catch(e)