-
-
Notifications
You must be signed in to change notification settings - Fork 242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for Node.js 8 native http2 #122
Comments
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. |
I'll probably give it a shot sometime soon. For now, I'm back to using the |
I've tested out these changes on my HTTP2 server and @michael42's suggestions seem to be working. PR: #155. Can you take a look, @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. |
There is currently a bug in the preview feature of Vite Issue 2754. The suggested fix Fix usage of undocumented _implicitHeader and _heade works fine, as forcing Chrome to use HTTP/1 with the command line flag 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? |
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. |
@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. |
Ok, that seems reasonable. So these are the options?
|
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. |
This comment has been minimized.
This comment has been minimized.
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. |
Can someone please clearly document the workaround? |
…ession` does not support HTTP/2 Reference: expressjs/compression#122 Also, separate `response.setHeader` calls from `response.end`, which were causing issues when using HTTPS.
Dropping some context from slack re: #170
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 💯 |
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 |
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:
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?The text was updated successfully, but these errors were encountered: