feat(#2458): WebSocket through HTTP/2

[metcoder95]

This relates to...

Closes #2458

Rationale

Implements WebSocket over HTTP/2

Note

Experimental warning is emitted on first usage.

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[KhafraDev]

I started looking into it too... perfect timing

[lpinca]

See websockets/ws#1458 (comment) for a simple/limited wrapper.

[metcoder95]

I found a bit of a wall here; for fetch, it seems the header expected is upgrade in order to understand the connection should be upgraded to a websocket. Nonetheless, this is not a valid H2 header, so we can just ack if we identify is an H2 connection or some other idea that we might have.

Ref: https://github.com/nodejs/undici/blob/be390d8cdd621a19da87ea65e0110f932394d72f/lib/web/websocket/connection.js

Happy to hear your thoughts @KhafraDev

[KhafraDev]

We check for the header because it's needed to complete the http upgrade and opening handshake (RFC 6455). For http2, we should follow RFC 8441.

[metcoder95]

@KhafraDev feel free to take another look

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.62011% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.84%. Comparing base (2fa4db2) to head (b6c3895).

Files with missing lines	Patch %	Lines
lib/dispatcher/client-h2.js	92.46%	11 Missing ⚠️
lib/web/websocket/connection.js	87.50%	3 Missing ⚠️
lib/web/fetch/index.js	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4540      +/-   ##
==========================================
- Coverage   92.86%   92.84%   -0.02%     
==========================================
  Files         106      106              
  Lines       33111    33272     +161     
==========================================
+ Hits        30749    30892     +143     
- Misses       2362     2380      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[metcoder95]

@KhafraDev feel free to have another look to the docs and let me know your thoughts 👍

[KhafraDev]

There seems to be a relevant test failure in test/http2-dispatcher.js

[KhafraDev]

Suggested change

	> 🗒️ Note: WebSocket over HTTP/2 is possible to be enabled by default in further major versions,
	> this will happen by enabling HTTP/2 connections as the de`fault behavior of Undici's Agent as well the global dispatcher.
	> Stay tuned to the changelog for more information.
	> 🗒️ Note: WebSocket over HTTP/2 may be enabled by default in a future version,
	> this will happen by enabling HTTP/2 connections as the default behavior of Undici's Agent as well the global dispatcher.
	> Stay tuned to the changelog for more information.

[KhafraDev]

Is the idea to enable http/2 by default outside of websocket? I was thinking of having it enabled only for websockets because I remember there was some push back last time you tried enabling it by default.

[metcoder95]

Not really sure what could be the plan here; the thing with enabling it only for WebSocket is that it might require to use a custom agent if H2 not enabled through global dispatcher as default.

It will be better to sync them, but if we are ok deviating one from the other, that should be totally fine

[metcoder95]

@KhafraDev can you check? Let me know if is ok to merge it in this way

[KhafraDev]

There are some http2 test failures, I'm not sure if they're relevant.

[metcoder95]

Seems that they are now fixed, the failures doesn't seem related to the PR's changes