-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix(Server): don't use spdy
on node >= v10.0.0
#1451
Conversation
/cc @SpaceK33z |
Codecov Report
@@ Coverage Diff @@
## master #1451 +/- ##
==========================================
+ Coverage 73.3% 74.02% +0.72%
==========================================
Files 10 10
Lines 663 666 +3
==========================================
+ Hits 486 493 +7
+ Misses 177 173 -4
Continue to review full report at Codecov.
|
Updated the test with an increased timeout because of the on-the-fly key generation. (Edit: And updated again to appease the linter.) |
Unfortunately, just changing the module to http2 won't work because of expressjs/express#3388 Express doesn't seem to actually use node's request/response stuff but uses custom ones which are then |
@Kovensky Without having looked into it deeper, from expressjs/express#3390 (comment) and below, it looks like there might be some movement in that area? :) |
Btw, are the recommendations for trying out pull requests in the CONTRIBUTING.md accurate? See e.g. nodejs/node#21665 (comment) |
Any news on this one ? |
/cc @Kovensky this PR don't fix problem? Can i look on original issue and/or minimum reproducible test repo? |
I think this is definitely blocked by nodejs/node#21665 and expressjs/express#3388 atm |
@evilebottnawi this branch itself should reproduce it; the hard part is automated testing. To reproduce it, just do an h2 connection to webpack-dev-server in @michael-ciniawsky "fixing" the node bug will just make the spdy module stop crashing but is not the real fix; either fixing the express bug or migrating away from express are. |
@Kovensky Yep, I understand it's blocked on that :) |
@michael-ciniawsky I’m not sure if I’m understanding you correctly, but this PR is not blocked on either of these issues. It fixes the first one, and the second one is just something to keep in mind for the future (fully migrating from SPDY to the built-in HTTP/2 module in Node would be dependent on it being solved). What this PR may be blocked on is this issue, reported by somebody who tried this code out: nodejs/node#21665 (comment) I don’t know enough about webpack-dev-server to figure out whether that issue is actually related, and if so, in what way. (And as @Kovensky said, automated testing is hard here & I’m not sure how to do it well.) |
spdy
on Node 10 spdy
on node >= v10.0.0
`spdy` is effectively unmaintained, and as a consequence of an implementation that extensively relies on Node’s non-public APIs, broken on Node 10 and above. In those cases, only https will be used for now. Once express supports Node's built-in HTTP/2 support, migrating over to that should be the best way to go. Fixes: webpack#1449 Fixes: nodejs/node#21665
I’ve rebased, and dropped the commit that adds Node 10 to the CI matrix since the .travis.yml has changed significantly (and I assume it would have been added anyway as part of that refactor if there was interest in it). |
This fixes the functionality of this dependency for Node 10 and above. Refs: webpack/webpack-dev-server#1451 Refs: nodejs/node#21665
For Bugs and Features; did you add new tests?
This does add Node 10 to the test matrix. There do not seem to be any existing tests for
https://
support, though, so I added one, even though it does not function as a regression test (one would actually have to explicitly use spdy/http2 for that).Motivation / Use-Case
This fixes the bug reported at:
Breaking Changes
This should not affect any user agents that detect protocol support themselves (like they should), in particular not browsers.
Additional Info
Another option that was considered was reverting the change that led to this in Node 10.
However, the
spdy
module is built in a very fragile manner, and unfortunately it seems unreasonable to expect that anything we can do inside of Node would be more than a stop-gap solution.(See e.g. spdy-http2/node-spdy#333 for an attempt to reach out to the
spdy
maintainers about this problematic.)