search

expressjs / compression

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

Bugfix/use write head instead of implicit header #170

Closed Icehunter closed 4 days ago

Icehunter commented 4 years ago

updated PR with rebase from express/compress:master to handle #128

dougwilson commented 4 years ago

Just wanted to note on here it looks like just a rebase, without addressing the review comments yet. I presume that will come in follow up commits?

Icehunter commented 4 years ago

Just wanted to note on here it looks like just a rebase, without addressing the review comments yet. I presume that will come in follow up commits?

Yes, working on that now.

Icehunter commented 4 years ago

@dougwilson I found all the comments I think needed to be fixed, except 2 perhaps that I'll work on tonight.

  1. the on('data') callback which I think doesn't need a data check; per the noop it's just a way to prevent hangs
  2. the comments from @sogaani regarding the response code. I'll look at that as well.
Icehunter commented 4 years ago

@dougwilson Is there anything more you see for this PR to have? @sogaani with regards to your comment on the error status code; it seems to be a big edge case. I work on a very large API and haven't encountered this issue before.

dougwilson commented 4 years ago

I was under the impression you were going to make changes in regards to the two bullet points above, which is what I'm waiting on. I never saw any additional comments after that message. Are you still working on them?

Icehunter commented 4 years ago

Eh! Sorry; I did leave the on('data') callback as is in the test, but I removed all the logs and I believe covered everything you had commented on in the previous test.

For the second bullet point I quite literally have no clue off the top of my head how to even cover that in an edge case test.

valoricDe commented 3 years ago

Seems like this one got stuck right?

lamweili commented 2 years ago

@Icehunter, you might want to correct the indentations at line 740-744 and line 747-753. I didn't include them previously so that https://github.com/expressjs/compression/pull/170/commits/38fe9fd5756f524e438c9abb9e91e7193921e0e3 is surgical.

https://github.com/expressjs/compression/blob/38fe9fd5756f524e438c9abb9e91e7193921e0e3/test/compression.js#L737-L756

lamweili commented 2 years ago

@Icehunter, for the clarity that #129 is not in this PR, the description should change:

updated PR with rebase from express/compress:master to handle #128 and #129

Icehunter commented 2 years ago

@Icehunter, for the clarity that #129 is not in this PR, the description should change:

updated PR with rebase from express/compress:master to handle #128 ~and #129~

Updated and rebased again :)

lamweili commented 2 years ago

@dougwilson When you are free, can you review this PR? Fixes #122 and supersedes #128. Many thanks! 🤗

I didn't have access in the forked repository to run the GH actions. So I forked the PR. I ran the GH actions on my own repository and here is the result for the latest commit (4c1d27b): https://github.com/lamweili/compression/actions

lamweili commented 2 years ago

@dougwilson, have you got the time to take a look at this PR?

bjohansebas commented 1 week ago

sorry @Icehunter, Can you fix the code style?

Icehunter commented 1 week ago

sorry @Icehunter, Can you fix the code style?

Sure thing.

jonchurch commented 1 week ago

Dropping some context from slack re: this PR

The original issue here is compression relying on node http internals since the days of connect in 2011

Here's when the commit w/ _implicitHeader originally landed:

https://github.com/senchalabs/connect/commit/715e4f56f8e4ad75baa35a7ae049bd69ca8bf6c6

http2 module has a different API and internals than http1 in node. So that method doesn't exist at all in http2, throwing when used.

Compression module doesn't support http2 explicitly. It likely can, but was never intended to. writeHead has been around since that first commit, but for reasons lost to time wasn't used

Idk that I can say that issue #122 "supporting http2" is closed after landing #170, because I personally haven't reviewed the code or tested for http2 edgecases.

However, removing an http internal to replace w/ a public interface which is equivalent is 💯

It doesn't look like _implicitHeader implementation has changed much since Node v0.4.12: https://github.com/nodejs/node/blob/v0.4.12/lib/http.js#L796

Here it is in v23: https://github.com/nodejs/node/blob/v23.1.0/lib/_http_server.js#L336

jonchurch commented 1 week ago

This history is long w/ 19 commits, @bjohansebas when/if you merge I'd suggest squashing via the dropdown to keep the history clean

history.md isn't updated in this PR, we've talked about dropping that file (at least blake is planning to do so) but just a reminder in case you want to make sure you don't forget changelog update