Ignore status message for HTTP/2

[genki]

To avoid setting status message that is not supported by HTTP/2, checked if the request's HTTP major version is less than 2.
Without this fix some of apps using this package shows warning as follows

(node:49588) UnsupportedWarning: Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)

[genki]

I found that in node 9.x an unexpected warning (as follows) had caught and failed the test.

(node:2676) ExperimentalWarning: The http2 module is an experimental API.

So I fixed the test to recognize the type of warning.

[genki]

@dougwilson Would you mind if I ask you to proceed the workflow?
I have completed to add the test and confirmed it gets succeeded in several environments.

[dougwilson]

Thanks! Sorry it was a holiday weekend. I'm not sure this module actually works with http2 yet, though, but landing this would imply it does. I'll need to check that out before we can land, to make sure http2 even works right. I wouldn't want people thinking it does with this and ending up with some kind of dos vector. Need to make sure the features all work and this module behaves correctly when it encounters broken pipes, half written reaponses, etc

[genki]

I am looking forward to have done it :)

[alexander-akait]

/cc @dougwilson Can you look at this? We are working on http2 support for webpack-dev-server (webpack/webpack-dev-server#5267) and we've done a huge amount of testing and everything works great except for the depcation output - UnsupportedWarning: Status message is not supported by HTTP/2 (RFC7540 8.1.2.4), We are planning to release soon which means many developers may encounter this problem and I am afraid you may be overwhelmed numbers of issues and comments. If we release a patch for this we can avoid the potential problem. Thank you

[wesleytodd]

Hey @alexander-akait, it looks like Doug left off with this checking how this package works with http2. Did you have any added insight into if the rest of the behaviors of this module work with http2? Would you be willing to add tests and stuff which prove it?

I don't think anyone is opposed to landing this, but with Doug having stepped back from the express project for now I didn't want to take action until I had time to investigate more. If you can assist with that I am sure we can make this happen.

[alexander-akait]

@wesleytodd Which test do you want to see?

Did you have any added insight into if the rest of the behaviors of this module work with http2?

I can't cover all possible scenarios correctly. But I make tests for a lot of scenaries and different middlewares - https://github.com/webpack/webpack-dev-server/tree/master/test, I honestly don't understand why the current solution is somehow dangerous. This is just an additional check, nothing more.

Technically we could add here typescript jsdocs and see any uncompatibilities properties/methods/etc, but in my opinion I didn't find anything incompatible with http2 module, we use these deps

var encodeUrl = require('encodeurl')
var escapeHtml = require('escape-html')
var onFinished = require('on-finished')
var parseUrl = require('parseurl')
var statuses = require('statuses')
var unpipe = require('unpipe')

and all of them is not related to http/https/http2 and even spdy and quic, just libraryes to parse/escape/callbacks/etc

We already have test here - https://github.com/pillarjs/finalhandler/pull/45/files#diff-a561630bb56b82342bc66697aee2ad96efddcbc9d150665abd6fb7ecb7c0ab2fR581 and it covers almost all lines, try to run it and look coverage, uncovered lines have nothing to do with server logic.

So to be honest I don't understand why it took so long...

As I said before, this is just an additional check, we don't necessarily have to declare full support for everything, if there are any bugs or requests for improvements, we can implement them in the next versions.

Thank you for your answer

[wesleytodd]

I honestly don't understand why the current solution is somehow dangerous.

I didn't say it was dangerous. I am just seeking to come up to speed on how you are using this with http2 to make sure this is the right decision.

Technically we could add here typescript jsdocs and see any uncompatibilities properties/methods/etc

That would not be my concern at all, it would be if the ordering of events (which is primarily what this package was built to address) results in correct runtime behavior. It has nothing to do with types or incompatible properties.

So to be honest I don't understand why it took so long..

This project is run by volunteers. It is large and gets TONS of issues and reports. Before earlier this year Doug was doing the herculean effort of all this by himself. Comments like this are not at all helpful and frankly not welcome here. Please do not downplay all of the free work Doug has done on this over the years.

As I said before, this is just an additional check, we don't necessarily have to declare full support for everything, if there are any bugs or requests for improvements, we can implement them in the next versions.

I totally agree with this.

---

So, back to the topic:

Which test do you want to see?

I would like to see at least one single test to indicate that this package works at all as expected with an http2 server. It doesnt need to do a whole lot, but just making sure the basic functionality works as intended.

[wesleytodd]

I guess to be clear after re-reading the test which was written, I would expect a test which covers more than just the 404 case. Ideally the "happy path" is tested.

[alexander-akait]

I guess to be clear after re-reading the test which was written, I would expect a test which covers more than just the 404 case. Ideally the "happy path" is tested.

👍 WIP

This project is run by volunteers. It is large and gets TONS of issues and reports. Before earlier this year Doug was doing the herculean effort of all this by himself. Comments like this are not at all helpful and frankly not welcome here. Please do not downplay all of the free work Doug has done on this over the years.

I said this not to offend anyone, but to understand the reason for the delay, because it is not clear from the top comments what is expected - more tests, documentation or something else, it just froze and that's it (#45 (comment)), and that's is why I wrote here. I also get hundreds of issues and questions and I understand perfectly well what volunteer work means.

[wesleytodd]

Sorry if I misunerstood the tone. I am also not sure if there were additional concerns Doug had. Let me know if my response above is enough to give you the detail you need to get this landed, by the comment about WIP for that test it sounds like you are good to go. I will attempt to follow up on this as quickly as I can to get this landed.

[wesleytodd]

Check out this one: https://github.com/pillarjs/finalhandler/pull/34/files#diff-a561630bb56b82342bc66697aee2ad96efddcbc9d150665abd6fb7ecb7c0ab2fR517-R545

If we could just combine all this it would be great. I got the CI fixed on master just now, so if we can get this into shape I can publish asap.

[alexander-akait]

@wesleytodd Done #53, I decide to make full testing to ensure all works fine