Support for Node.js 8 native http2

[michael42]

Sorry for opening another issue on this topic (#77, #78), but I'm trying out the new native http2 support in Node.js and compression (1.7.1) fails with:

TypeError: this._implicitHeader is not a function
    at Http2ServerResponse.write (node_modules/compression/index.js:84:14)

As far as I see it, this method isn't part of the public API of http.ServerResponse. Shouldn't compression simply call if (!this.headersSent) this.writeHead(this.statusCode) (in a helper function) instead of depending on undocumented API?

[dougwilson]

I don't think this is a duplicate issue, since those other ones are referring to something completely different than the http2 that is part of Node.js 8 (and of course being over a year old they would have neeed to br able to see the future to refer to Node.js 8).

If that chabge works, then absolutely feel free to make a PR with the change! If it's possible to use a public API that is definitely best regardless of http2 support.

[michael42]

I'll probably give it a shot sometime soon. For now, I'm back to using the spdy module, because of http-proxy-middleware issues and random uncaughtException events.

[DullReferenceException]

I've tested out these changes on my HTTP2 server and @michael42's suggestions seem to be working. PR: https://github.com/expressjs/compression/pull/155. Can you take a look, @dougwilson?

[karlhorky]

I guess #128 is the active pull request (#155 was marked as a duplicate of it). It has recently been updated (16 hours ago, at the time of writing).

[dougwilson]

So #155 either needs to be closed as a duplicate of the issue I referenced in it, or someone should add information as to how it is not a duplicate / add tests to that PR if the desire is to land. I'm sure the change fixes / does something, so it doe need associated tests at minimum, or whatever it is trying to accomplish will not continue to work into the future.

[gunters63]

There is currently a bug in the preview feature of Vite Issue 2754. As soon as Node and the browser both agree on using HTTP/2 the preview server crashes with TypeError: this._implicitHeader is not a function because it uses express and compression.

The suggested fix Fix usage of undocumented _implicitHeader and _heade works fine, as forcing Chrome to use HTTP/1 with the command line flag --disable-http2.

The latter is not an option, though, because some browser apps depend on HTTP/2. Disabling compression in Vite is also not a good idea, because the preview feature wants to be close to a production environment.

So what is the supposed solution here? I don't understand why the proposed fix was never merged. The current code uses undocumented API calls specific to nodes HTTP/1 code path.

If there are really outstanding stability issues in some platform/environments, couldn't they be tracked in a new issue?

[dougwilson]

Hi @gunters63 the main issue is this module (and Express 4.x, in which it lives) was never designed for the Node.js http2 module. The only http/2 support it had was through the spdy module. The main issues run into is that the proposed fix seems fine on the surface, but only if you are trying to just make it not hard crash. There are various subtle bugs, for example responses not fully completing, etc. that occur when this module gets put on http2.

[dougwilson]

@gunters63 as for the PR you referenced, the author of the PR closed the PR themselves. I presume because they were never able to get it to pass CI (based on the old convo in there), but they didn't say the specific reason why they closed it, so I can only speculate. Closed PRs cannot be merged by the source project.

[gunters63]

Ok, that seems reasonable.

So these are the options?

• Disable compression in vite (for a short term solution, make it configurable with default disabled)
• Wait for Express 5
• Move to a different HTTP server (Koa, Fastify)

[dougwilson]

Hi @gunters63 I'm not familiar with vite, so I cannot comment on that one, at least. As far as waiting for Express 5, that shouldn't be required. These middlewares are separate so they have their own release schedule. I was only providing some history. As for changing to a different HTTP server, I think all of those use the Node.js http and http2 modules so I would assume you'd still have the same issue, as this module doesn't work on http2, currently.

[gunters63]

Hi @dougwilson.

At least Fastify and Koa seem to have their own compression module. Fastify offers experimental support for HTTP2. Not sure about Koas HTTP/2 status.

I am forced to use HTTP/2 even during development because my client code uses gRPC. I think my safest bet for now is patching Vite to disable compression (I already asked the Vite team to make compression configurable). Its not that important as I rarely use the preview feature. The problem does not exist for the development server.

[DullReferenceException]

The main issues run into is that the proposed fix seems fine on the surface, but only if you are trying to just make it not hard crash. There are various subtle bugs, for example responses not fully completing, etc. that occur when this module gets put on http2.

The responses not completing may relate to this http2 bug in node which has now been fixed; we've been using the hack for H2 for some time and it seems to work.

[ghost]

Can someone please clearly document the workaround?

[jonchurch]

Dropping some context from slack re: #170

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 this issue "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 💯

[bjohansebas]

Yes, there are still tests to be done to say that we fully support http2. If anyone wants to continue with this, that would be great, otherwise, I’ll add more tests for http2 in the future