Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Exp: HTTP/2 #2099

Closed
wants to merge 15 commits into from
Closed

Exp: HTTP/2 #2099

wants to merge 15 commits into from

Conversation

RobinTail
Copy link
Owner

@RobinTail RobinTail commented Oct 15, 2024 •
edited
Loading

Experiments regarding #2092

  • h2 is HTTP/2 over TLS (protocol negotiation via ALPN)
  • no browsers supporting HTTP/2 without encryption
  • Undici Agent option: allowH2

@RobinTail RobinTail added enhancement New feature or request miracle Mysterious events are happening here labels Oct 15, 2024
@RobinTail RobinTail mentioned this pull request Oct 15, 2024
Merged
@RobinTail RobinTail added this to the v21 milestone Oct 15, 2024
@RobinTail RobinTail mentioned this pull request Oct 15, 2024
Merged
RobinTail added a commit that referenced this pull request Oct 15, 2024
@RobinTail
Return generic array of servers (#2100)
024411f 
This should ease declaration and enable future improvements like #2099 

Making `createServer()` to return an object having `servers` property
instead of `httpServer` and `httpsServer`
@coveralls-official Coveralls Official
Copy link

coveralls-official bot commented Oct 15, 2024 •
edited
Loading

Coverage Status

coverage: 100.0%. remained the same
when pulling f8f223f on try-http2
into 5ca76bc on make-v21.

RobinTail
RobinTail commented Oct 15, 2024
View reviewed changes
tests/express-mock.ts Outdated Show resolved Hide resolved
RobinTail
RobinTail commented Oct 15, 2024
View reviewed changes
yarn.lock Outdated Show resolved Hide resolved
RobinTail added a commit that referenced this pull request Oct 15, 2024
@RobinTail
Cherry-picking express mock from #2099.
cae8246
@RobinTail RobinTail mentioned this pull request Oct 15, 2024
Merged
RobinTail added a commit that referenced this pull request Oct 15, 2024
@RobinTail
Better express mock (#2103)
dfb2e3a 
Cherry-picked from #2099
@RobinTail
Copy link
Owner Author

RobinTail commented Oct 16, 2024 •
edited
Loading

Crashes with

TypeError: Cannot read properties of undefined (reading 'readable')
    at IncomingMessage._read (node:_http_incoming:214:19)
    at Readable.read (node:internal/streams/readable:737:12)
    at resume_ (node:internal/streams/readable:1255:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
Emitted 'error' event on IncomingMessage instance at:
    at emitErrorNT (node:internal/streams/destroy:169:8)
    at errorOrDestroy (node:internal/streams/destroy:238:7)
    at Readable.read (node:internal/streams/readable:739:7)
    at resume_ (node:internal/streams/readable:1255:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Something is messed up with a prototype chain.
This seems to be a compatibility issue with Node >= 15, the bridge and spdy as well:

https://github.com/expressjs/express/issues/3388
https://github.com/rahulramesha/http2-express-bridge/issues/11
https://github.com/spdy-http2/node-spdy/issues/380

@RobinTail RobinTail added external bug it's a bug, but in a dependency dependencies Pull requests that update a dependency file labels Oct 16, 2024
@RobinTail
Copy link
Owner Author

RobinTail commented Oct 16, 2024 •
edited
Loading

  • spdy, and spdy-fixes — works, but:
    • corrupts prototypes for HTTP server — some side effects that makes it impossible to run together, it must be exclusive one

UPD: found a way to use spdy-fixes conditionally, using same pair of servers

RobinTail
RobinTail commented Oct 16, 2024
View reviewed changes
yarn.lock Show resolved Hide resolved
RobinTail
RobinTail commented Oct 16, 2024
View reviewed changes
yarn.lock
@@ -2277,6 +2309,11 @@ is-stream@^2.0.0:
resolved "https://registry.yarnpkg.com/is-stream/-/is-stream-2.0.1.tgz#fac1e3d53b97ad5a9d0ae9cef2389f5810a5c077"
integrity sha512-hFoiJiTl63nn+kstHGBtewWSKnQLpyb155KHheA1l39uvtO9nWIop1p3udqPcUd/xbF1VLMO4n7OI6p7RbngDg==

isarray@~1.0.0:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ancient sheet

RobinTail
RobinTail commented Oct 20, 2024
View reviewed changes
yarn.lock
html-escaper@^2.0.0:
version "2.0.2"
resolved "https://registry.yarnpkg.com/html-escaper/-/html-escaper-2.0.2.tgz#dfd60027da36a36dfcbe236262c00a5822681453"
integrity sha512-H2iMtd0I4Mt5eYiapRdIDjp+XzelXQ0tFE4JS7YFwFevXXMmOp9myNrUvCg0D6ws8iqkRPBfKHgbwig1SmlLfg==

http-deceiver-fixes@^1.2.8:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this thing is using low level API of process.bindings that was removed in Node 14.
I'm surprised that it still works anyhow.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked it on Node 20 and despite the Node.js documentation the method is still present ¯\_(ツ)_/¯

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyway, that low-level approach won't work anywhere else like Deno/Bun.
we'll have to way another year for express team to make version 6 that will support http2 natively

@RobinTail
Copy link
Owner Author

#2099 (comment)

@RobinTail RobinTail closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request external bug it's a bug, but in a dependency miracle Mysterious events are happening here
Projects
None yet
Milestone
v21
Development

Successfully merging this pull request may close these issues.
1 participant
@RobinTail