remove warning when used with HTTP2

[tellnes]

The HTTP2 spec does not support sending a status message, so when res.statusMessage is set a process warning is emitted.

[dougwilson]

Thank you so much for this! I have a couple things to quest for this change:

1. None of the functionality of this module has been tested in the Node.js HTTP2 server, and I see that this is to make it work in HTTP2, but doesn't actually run any of our existing test suite in the HTTP2 server, to ensure all the other features work / keep working. I would suggest that all the test suite gets run under HTTP2 in order to accept in HTTP2-specific changes.
2. Along with that, it should likely be documented around the HTTP2 support, and which of the HTTP2 servers in Node.js it supports (I believe there are two different ones).
3. The documentation in the README now seems to no longer reflect the actions this module is taking, so likely want to just update that as well.

Thank you!

[tellnes]

Thank you for a quick response.

After a quick look it does not look like supertest supports HTTP2. Like you said we do want to run the test suite both for http and http2. So to be able to run the test suite under http2, we probably have to do a big refactor or rewrite of the test suite. But it would be great if there is some quick clever way to do this that I'm not seeing.

Yes there is two ways to do HTTP2 in core. I've assumed usage of the compatibility API in this pr. The HTTP2 Compatibility API targets the public API of the http module. The more low level API exposed by core is not compatible with this module without bigger changes. Eg. there is no request and response object, but a duplex stream instead.

I've read trough index.js and I think that we should not have any other problems than what is in this pr. But yes, it should of course be tested.

I think a bigger rewrite of the test suite is out of scope for this pr and probably something that I also don't have time to do. But I could add something to the readme that document current HTTP2 support.

[dougwilson]

Yep, totally understandable, but I hope it it also understandable that your PR is actually adding HTTP2 support by the nature of this change, which is why, IMO, it is not out of scope to actually get it testing under HTTP2. If the goal of this PR is not to get this module to actually support HTTP2, then what is it? Perhaps I'm missing the goal of this PR altogether 😆 . Right now it doesn't support HTTP2, so it is understandable there are issues with it. But I think you're trying to fix that by in essence, adding HTTP2 support, which would mean we should actually add HTTP2 support. I hope that makes sense.

[tellnes]

I'm having this problem because I'm using the http2 module. My goal is to fix that problem. So yes, you can say the goal is to add HTTP2 support.

Actually, I don't really understand why this module is changing this property. Is the goal to reset it in case it was set in a previous middleware?

If that is a case, another solution might be to just set res.statusMessage to an empty string. Core will set the correct status message) if it is not customized and has been doing that since at least 0.8.x which is the oldest version this library is currently tested on.

Btw, core also exposes the status codes as http.STATUS_CODES so the statuses module is not needed.

[dougwilson]

Actually, I don't really understand why this module is changing this property. Is the goal to reset it in case it was set in a previous middleware?

Yep, that is the main purpose. The other purpose was also just to make sure that a specific version of this module would always set the status message to the same value no matter the Node.js version in use (which is also why it uses statuses -- not all the older Node.js versions this module supports has all the same codes and messages; 415 is an example off the top of my head).

If your solution works for this and also just happens to make this warning go away from HTTP2, that is A-OK with me. But the current changes are very explicit about HTTP2, and I think it would be a really disservice to folks who see that thinking this module has strong HTTP2 when it does not; we should add in HTTP2 right instead of just trying to hack in it :) At least, that is the bar at which the modules in this organization are managed, if that makes sense.

[tellnes]

My idea was

if (res.statusMessage) {
  res.statusMessage = ''
}

But it does not work since node core also warns when you are reading the property. Here is the source code in node that we need to work around: https://github.com/nodejs/node/blob/v14.5.0/lib/internal/http2/compat.js#L608-L616

So I don't think we can get around having a check if the response object is from HTTP2 or not.

Btw, I do consider this module HTTP2 compatible even without this change. It just prints an annoying warning to the console. All modules written for the http module should also be compatible with the HTTP2 Compatibility API as long as they only use public APIs from core and not any internal ones.

[dougwilson]

All modules written for the http module should also be compatible with the HTTP2 Compatibility API as long as they only use public APIs from core and not any internal ones.

Yea, and that is a good point, though: you're actually identifying perhaps an issue with the compatibility API, right? Because if now in order to have something that working using purely only the public APIs (such as the case here) of the HTTP/1 module, then it should work with HTTP/2 without needed to introduce forks in the code branches and HTTP/2-specific changes, right? So perhaps this is actually an issue in the HTTP/2 compat API itself? For example, why does it even bother with the warning? That does make it hard to just write code once for use in both HTTP/1 and HTTP/2, no?

[tellnes]

Yes, I was actually thinking the same after writing my latest comment. But I'm also thinking that the warning from core is the correct behavior. HTTP/2 has removed support for status messages. If node core where just to ignore it, it can confuse developers who expect it to behave like HTTP/1. So the solution is to stop using it in user land since it is a deprecated feature.

What we need is some API that lets us check if it is set without triggering the warning. Maybe the middle ground here is to change core to not emit the warning in the getter, but still emit it in the setter.

Btw, I've never seen this feature used for anything in the real world. Have you?

[dougwilson]

Yes, I was actually thinking the same after writing my latest comment. But I'm also thinking that the warning from core is the correct behavior. HTTP/2 has removed support for status messages. If node core where just to ignore it, it can confuse developers who expect it to behave like HTTP/1. So the solution is to stop using it in user land since it is a deprecated feature.

Yea, and I can see it both ways. I think the question is really to Node.js core and what their goal of the HTTP/2 compat API actually is. If it is to allow users to make a single program that works with HTTP/1 and HTTP/2 without needing to constantly sniff the HTTP version and other aspects of the request, I would argue that it should not emit the warning, and just ignore it if it doesn't do anything in HTTP/2. But maybe that is not their goal, which would then mean you assumption that this module works correctly with the HTTP/2 compat API may be built off an assumption there and we really do need to explicitly test it and ensure that it works as expected, including not emitting warnings that are HTTP/2-specific.

What we need is some API that lets us check if it is set without triggering the warning. Maybe the middle ground here is to change core to not emit the warning in the getter, but still emit it in the setter.

Yea, it's certainly possible. The Node.js project would need to be engaged to determine that. I was also looking at it seems the value it defaults to is an empty string, which HTTP/1 defaults to undefined -- that is a weird inconsistency between the two APIs, and those kind of subtle differences makes it all the much more important that if this module is going to make HTTP/2s-specific changes, we need to fully test it against that API, as it seems there is various differences between the two of them.

Btw, I've never seen this feature used for anything in the real world. Have you?

Which feature, specifically? The change here was made due to user request, so I assume there are users who are using it. I have not used it myself, but then again, if someone came along asking about your change, I would have the same answer: someone asked for it, but beyond that, I don't have any more details :)