http: allow passing server object, for future node http2

[fl0w]

Just trying to get this ball rolling because there's some activity in closed issue #477.

This is what I'm purposing to allow user space to experiment with http2 without breaking backwards compatibility.

Purposed API would become:

const http2 = require('http2')
const Koa = require('koa')
const app = new Koa({ server: http2 })

I could move the require into the construction if args.server is falsey to avoid loading the library at all if http2 is chosen - not sure about the implications of doing so. I'd still have to be done during initialisation of a Koa application so my two cents would be to do it. Not sure how Koa team feels about a require in a constructor thought :)

constructor() {
  super()
  ...
  this.server = args.server || require('http');
  ...
}

PS I am embarrassingly ignorant about http2, and the node implementation.

[codecov]

Codecov Report

Merging #1063 into master will decrease coverage by 0.79%.
The diff coverage is 57.14%.

@@            Coverage Diff            @@
##           master    #1063     +/-   ##
=========================================
- Coverage   99.72%   98.93%   -0.8%     
=========================================
  Files           5        5             
  Lines         369      374      +5     
=========================================
+ Hits          368      370      +2     
- Misses          1        4      +3

Impacted Files	Coverage Δ
lib/application.js	97.02% <57.14%> (-2.98%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0168fd8...e244414. Read the comment docs.

[likegun]

Would it be better with:

const Koa = require('koa')
const app = new Koa({ server: 'http2' })

this.server = require(args.server == 'http2' ? 'http2' : 'http');

Otherwise if people pass wrong server value to constructor,they will get error like "this.server.createServer is not a function" and feel confused.

[iyuq]

I don't think this is necessary. We can simply use http2 in the following way.

const Koa = require('koa');
const app = new Koa();

const http2= require('http2');
http2.createServer(app.callback()).listen(...);

[fl0w]

@iyuq I agree with you, but that sort of opens the can for me :) In that case Koa should be completely agnostic and even remove the koa#Application.listen sugar (IMO) and thus de-depend remove require('http')

[dead-horse]

I think const app = new Koa({ server: 'http2' }) is better, koa#Application.listen sugar no need to require http or http2 manually. so we can create a server by:

new Koa({ server: 'http' }).listen();
new Koa({ server: 'http2' }).listen();
require('http').createServer(new Koa().callback()).listen();
require('http2').createServer(new Koa().callback()).listen();

[fl0w]

@dead-horse is the update better? This would allow either { server: 'http' } or { server: 'http2' }, or a custom http/http2 compliant implementation { server: require('custom-spdy-server') }.

I'll add tests and docs if this approach is acceptable.

[dead-horse]

LGTM

[jonathanong]

i'd rather keep .listen() minimal. people should be understanding that app.callback() just returns a function that you can use in any http server

[fl0w]

Closing because of bloat.

Update: Clarification
Because this introduced init options for koa#Application and bloats Application.listen() this PR was closed according to voiced concerns by owners.

If you want to use node.js core http2 module, feed it using app.callback() which works out of the box.

require('http2').createSecureServer(cert, new Koa().callback()).listen()

[Rukeith]

@fl0w What is the finally solution?
Use const app = new Koa({ server: 'http2' }) ?
I didn't find anything about this in document

[joshbeckman]

@Rukeith I believe this was closed in favor of using app.callback(), like in #1063 (comment)

[felixsanz]

But... what if you want to use both http2 and http1? You need to create 2 separate instances of Koa?

[fl0w]

@felixsanz yes - that would be the case regardless of this PR