Implemented http2 protocol support

PhilippSoehnlein commented 8 years ago

I did the wiring between node-http2 and grunt-contrib-connect, because I have the same need the author of #202 has.

Some comments / explanations regarding the PR:

To keep the changes in tasks/connect.js small, it currently depends on another PR I made for node-http2. If this PR takes too long to be accepted or will not be accepted at all the changes in this PR are very small (the server.listen().on().on() must be changed to three single statements: server.listen(); server.on(); server.on()).
Because node-http2 doesn't support the server.address() method yet (see molnarg/node-http2#157) I had to refactor a bit and removed the use of the function and replaced the result (from which only the port is used) with the foundPort variable which IMO has the same value and is in context anyway.
I had to set the minimum node version to 0.12, because of molnarg/node-http2#101. If support for 0.10 is still important, I suggest to add a check for node < 0.12 and protocol http2 which results in an error.

vladikoff commented 8 years ago

I had to set the minimum node version to 0.12, because of molnarg/node-http2#101. If support for 0.10 is still important, I suggest to add a check for node < 0.12 and protocol http2 which results in an error.

Yeah let's do that :+1:

vladikoff commented 8 years ago

Awesome work @PhilippSoehnlein, the windows build fail seems unrelated. Let's adjust 0.10 support and should be ready to be merged

PhilippSoehnlein commented 8 years ago

I implemented the check.

And fixed the test for the http2 implementation. Now the test fails, which is correct, because of the missing merge from https://github.com/molnarg/node-http2/pull/168.

PhilippSoehnlein commented 8 years ago

Well it looks like the folks at node-http2 aren't as responsive as the folks here are… What do you think? Should we go on waiting or should we adjust our code as described above?

vladikoff commented 8 years ago

Well it looks like the folks at node-http2 aren't as responsive as the folks here are… What do you think? Should we go on waiting or should we adjust our code as described above?

We can just fork that project and use it from our org. one sec...

vladikoff commented 8 years ago

@PhilippSoehnlein can you push your branch to https://github.com/gruntjs/node-http2, name the branch as : "fix-return-value". Then updated this PR to use the branch from github, then it should be ready

PhilippSoehnlein commented 8 years ago

Then updated this PR to use the branch from github

Sorry for being dumb here, but how can I do that?

vladikoff commented 8 years ago

@PhilippSoehnlein npm i gruntjs/node-http2#fix-return-value --save

vladikoff commented 8 years ago

@PhilippSoehnlein after you push to the fork I created you should be able to install it ^

vladikoff commented 8 years ago

@PhilippSoehnlein almost correct, seems like the format you set it with is not compatible with node 0.10:

No compatible version found: http2@'github:gruntjs/node-http2#fix-return-value'

vladikoff commented 8 years ago

@PhilippSoehnlein try something like this to be safe: https://github.com/mozilla/fxa-content-server/blob/master/package.json#L101

PhilippSoehnlein commented 8 years ago

Thanks for the help!

I also reviewed the unit test and noticed it was only partly implemented. This is fixed, but the thing now is, that tests will fail on node 0.10, because of the exception we agreed on when HTTP/2 is used with node 0.10. The only thing I can think of is to build the object of connect servers in Gruntfile.js in a more dynamic way (and not inline) and also patch the test in connect_test.js to ~~await an exception~~ be skipped under node 0.10. What do you think?

vladikoff commented 8 years ago

@PhilippSoehnlein we can say that http2 support is only available in node 0.12+

gruntjs / grunt-contrib-connect

Implemented http2 protocol support #203