search

expressjs / compression

Node.js compression middleware
MIT License
2.77k stars 241 forks source link

Fix usage of undocumented _implicitHeader and _header #127

Closed maritz closed 6 years ago

maritz commented 6 years ago

This fixes usage of compression with the http2 node core module where _implicitHeader is not implemented.

Fixes #122 and uses @michael42's proposed solution.

It matches the current _implicitHeader implementation.

I have also changed _header, because while http2 does provide a compat layer, I can't see the reason for using that instead of headersSent. I assume it's there for historical reasons, but headersSent was implemented in v0.9.3, so it should be fine now.

Tests run fine and in my test with an actual http2 server it also works.

dougwilson commented 6 years ago

This is awesome! I'll heck it out ASAP. I see the tests no longer pass on Node.js 0.8. Can you add back 0.8 support? If it's not possible, that's OK too, let me know. We can then hold this PR for inclusion in the next major release instead of releasing right now 👍

dougwilson commented 6 years ago

It would also be really awesome if you can add http2 tests (i.e. add tests that actually require this change, such that we don't accidentally regress the behavior).

maritz commented 6 years ago

Hm, we could go back to the usage of the undocumented _headers field instead of headersSent for 0.8.x and then make the headersSent patch seperate for the next major release.

http2 tests would either require creating temporary certificates (not sure if that works on travis, as it requires making a call to openssl afaik) or have a plain server/client, which I'm not sure how to do on the client side right now. A quick glance at the docs did not reveal an obvious way for that.

dougwilson commented 6 years ago

Hm, we could go back to the usage of the undocumented _headers field instead of headersSent for 0.8.x and then make the headersSent patch seperate for the next major release.

Which ever approach you're willing to do 👍 I'm fine with any choice, just wanted to let you know in case you wanted to make a change to the PR or just wait for a while for a merge while we get a new major planned.

http2 tests would either require creating temporary certificates (not sure if that works on travis, as it requires making a call to openssl afaik) or have a plain server/client, which I'm not sure how to do on the client side right now. A quick glance at the docs did not reveal an obvious way for that.

Yea, I've never done it before, so don't have any pointers. HTTP/2 works just fine over non-SSL in Node.js, I wouldn't think you need to do anything with SSL to test over HTTP/2.

We really don't accept changes without some kind of backing test, because most changes comes from pull requests, and many times when people contribute, they cannot know all the requirements in order to make a change, and it will take a long delay if someone had to manually comb through it every time. For example, you may have missed the Node.js 0.8 support requirement (assuming since you didn't mention in the PR that it would break that on purpose -- sorry if the assumption is wrong), but thankfully our Travis CI actually checks this, so we don't have to manually remember to check for the support. This is how all features should work in these projects 👍

maritz commented 6 years ago

Yeah, I really didn't expect 0.8 to be a target anymore. :-D

I'll split it into 2 PRs and I'll do some trial-and-error for the tests.

maritz commented 6 years ago

Opened #128 and #129 as discussed.