Bugfix/use write head instead of implicit header

[Icehunter]

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

[dougwilson]

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]

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]

@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]

@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]

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]

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]

Seems like this one got stuck right?

[lamweili]

Hi @Icehunter and @dougwilson,

IMHO, there are two issues covered by this PR.

1. Switching from an undocumented API to a public API.
   This is definitely preferred. Current tests should suffice as there are no logical changes.
2. Supporting http2 as a by-product.
   Additional tests required to prove support on http2.

I addressed the on('data') callback as mentioned in #128 (comment) and #170 (comment) with the following suggestions.
It now asserts that the data is correct.

Go to the Files tab and Add suggestion to batch so that it becomes a singular commit.
You might also want to correct the indentations within the function closeHttp2 at line 733.

I believe with this, all outstanding review comments in #128 are addressed and this PR should be ready and supersede #128.

[lamweili]

@Icehunter, you might want to correct the indentations at line 740-744 and line 747-753.
I didn't include them previously so that 38fe9fd is surgical.

compression/test/compression.js

Lines 737 to 756 in 38fe9fd

	function closeHttp2 (client, server, callback) {
	if (typeof client.shutdown === 'function') {
	// this is the node v8.x way of closing the connections
	client.shutdown({}, function () {
	server.close(function () {
	callback()
	})
	})
	} else {
	// this is the node v9.x onwards way of closing the connections
	client.close(function () {
	// force existing connections to time out after 1ms.
	// this is done to force the server to close in some cases where it wouldn't do it otherwise.
	server.close(function () {
	callback()
	})
	})
	}
	}

[lamweili]

@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]

@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]

@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]

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

[lamweili]

LGTM

(can't approve cause I don't have explicit access to this repository)
😮‍💨

[bjohansebas]

sorry @Icehunter, Can you fix the code style?

[Icehunter]

sorry @Icehunter, Can you fix the code style?

Sure thing.

[jonchurch]

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:

senchalabs/connect@715e4f5

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]

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