search

spdy-http2 / node-spdy

SPDY server on Node.js
2.8k stars 196 forks source link

Fix2 for express compression module not activating #202

Closed hdf closed 9 years ago

hdf commented 9 years ago

Fix for Issues:

199

and expressjs/compression#38

indutny commented 9 years ago

Yay, looking fantastic! :) Btw, do you know that you could just push to the same branch and the PR will automatically update with a new commits (even on a force push)? ;)

Now comes the second part of any change ;) It needs to have a test. To keep things simple I'd just copy on of the tests there: https://github.com/indutny/node-spdy/blob/master/test/unit/connect-test.js and add the headers test to it. May I ask you to do it?

hdf commented 9 years ago

No, I don't have much experience with git. Is this what you meant? Is it not a problem, that the req.rawHeaders will not be modified this way? It seems that req.rawHeaders isn't copied over to state.

indutny commented 9 years ago

Huuuh? Impossible!

hdf commented 9 years ago

Ah, I see, I don't need to set rawHeaders if I set state.headers['accept-encoding'] in stream.js. Although I don't understand why, but it is clear now, that that is the better place for this snippet. :) Not sure, what should be tested for exactly. Honestly I have never written a test before. :P

indutny commented 9 years ago

One minor nit, otherwise LGTM!

hdf commented 9 years ago

Github editor does not tell me where the 80 columns end.. :(

hdf commented 9 years ago

It is exactly 79 characters long now. :) You sure?

hdf commented 9 years ago

Maybe I should now write the version bump as well in to package.json? :)

indutny commented 9 years ago

@hdf landed in https://github.com/indutny/node-spdy/commit/a9a8069592f3c36b82b22518aa6613f77311cb98, thank you!