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?