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.

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

http: expose websocket in nodehttp #53721

Conversation

anfibiacreativa
Copy link
Contributor

This PR exposes Websocket in node:http as requested in #53684
Once we discuss, if it satisfies the requirement, we can backport to Node.js 20 and 22

CC: @mcollina @nodejs/http @manekinekko

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jul 4, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good job! Docs are missing too.

Can you add a test that the given objects are the same of the global?

lib/http.js Outdated Show resolved Hide resolved
@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 4, 2024
@RedYetiDev RedYetiDev added the lts-watch-v20.x PRs that may need to be released in v20.x label Jul 4, 2024
@RedYetiDev
Copy link
Member

Once we discuss, if it satisfies the requirement, we can backport to Node.js 20 and 22

I've added lts-watch-v20.x to the PR, as it may need to be backported to v20, but feel free to remove the label if you disagree.

test/parallel/test-http-import-websocket.js Outdated Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev self-requested a review July 4, 2024 21:21
Copy link
Member

@RedYetiDev RedYetiDev left a comment

Choose a reason for hiding this comment

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

Looks good, just needs some documentation, but from (mostly, except minor changes) here-on-out, I'll leave the reviewing to collaborators.

@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 5, 2024
@PuruVJ
Copy link

PuruVJ commented Jul 5, 2024

Thank you @anfibiacreativa for implementing this so darn fast! 🚀🙏

@manekinekko
Copy link
Contributor

As discussed in #53684, can we track this change to be backported to v22.x as well?

@anfibiacreativa
Copy link
Contributor Author

As discussed in #53684, can we track this change to be backported to v22.x as well?

Yes, @mcollina should we do the backporting in the same PR?

@RedYetiDev
Copy link
Member

As discussed in #53684, can we track this change to be backported to v22.x as well?

Once this lands, backport-requested-... labels can be added if needed.

There's no LTS watch label for v22.x yet.

@mcollina
Copy link
Member

mcollina commented Jul 5, 2024

As discussed in #53684, can we track this change to be backported to v22.x as well?

This will be released in v22 first, then backported to v20.x after at least two weeks.

@lpinca
Copy link
Member

lpinca commented Jul 5, 2024

It may be the most appropriate module, but why http? WebSocket is a different protocol and the implementation uses undici under the hood which shares very little/no code with the Node.js http module.

We don't expose fetch, Request, Response, etc. in the http module.

@RedYetiDev
Copy link
Member

What would you recommend exposing it under (if it were to be exposed at all), net?

@lpinca
Copy link
Member

lpinca commented Jul 5, 2024

What would you recommend exposing it under (if it were to be exposed at all), net?

Just like other new globals I would not backport it. Anyway, that is my personal opinion, not a blocker.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@manekinekko
Copy link
Contributor

lgtm

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 8, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 8, 2024
@nodejs-github-bot nodejs-github-bot merged commit a1869fa into nodejs:main Jul 8, 2024
54 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a1869fa

@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Jul 8, 2024
@manekinekko
Copy link
Contributor

Congrats @anfibiacreativa on your 1st contribution to node core!

Thank you all for taking the time to review these changes.

aduh95 pushed a commit that referenced this pull request Jul 12, 2024
PR-URL: #53721
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit that referenced this pull request Jul 12, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
module:
  * add __esModule to require()'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
worker:
  * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682

PR-URL: TODO
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
aduh95 added a commit that referenced this pull request Jul 12, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
module:
  * add __esModule to require()'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
worker:
  * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682

PR-URL: #53826
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
PR-URL: #53721
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit that referenced this pull request Jul 16, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 16, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 16, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 17, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
RafaelGSS pushed a commit that referenced this pull request Jul 17, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) nodejs#53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) nodejs#53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) nodejs#52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) nodejs#52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) nodejs#53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) nodejs#53462
test_runner:
  * support glob matching coverage files (Aviv Keller) nodejs#53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) nodejs#53682

PR-URL: nodejs#53826
@MTyson
Copy link

MTyson commented Jul 20, 2024

Is there any info on this yet? It looks like, based on the code, that it is just exposing undici lib and that is just a client, not a server?

@mcollina
Copy link
Member

This will be released in the next version of Node v22.

We don't plan to add a server.

@mcollina
Copy link
Member

@marco-ippolito @nodejs/releasers I saw this was not picked up by the latest 20.17. How can we make sure it's picked up in the next one?

@marco-ippolito
Copy link
Member

@marco-ippolito @nodejs/releasers I saw this was not picked up by the latest 20.17. How can we make sure it's picked up in the next one?

Yes, I was unsure because we didnt unflag websocket client on v20

@mcollina
Copy link
Member

The whole point of this is exposing the Websocket client in v20. It's written in the linked issue.

Can you make sure it's in the next one?

@KhafraDev
Copy link
Member

WebSocket can be exposed in node v20 (at least in 20.17.0) with --experimental-websocket as a global

@targos
Copy link
Member

targos commented Sep 21, 2024

This lands cleanly on v20.x-staging but the test doesn't pass:

  • It needs --experimental-websocket
  • Even with the flag, assert.strictEqual(NodeHttpMessageEvent, MessageEvent); fails because the global MessageEvent in v20.x is not the undici one.

@targos targos added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. lts-watch-v20.x PRs that may need to be released in v20.x needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.