compression
compression copied to clipboard
expressjs
Bugfix/use write head instead of implicit header
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?
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.
@dougwilson I found all the comments I think needed to be fixed, except 2 perhaps that I'll work on tonight.
- the on('data') callback which I think doesn't need a data check; per the noop it's just a way to prevent hangs
- the comments from @sogaani regarding the response code. I'll look at that as well.
@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.
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?
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.
@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
@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, 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 :)
@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