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

http: expose websocket in nodehttp #53721

Merged
merged 27 commits into from
Jul 8, 2024
Merged

http: expose websocket in nodehttp #53721

merged 27 commits into from
Jul 8, 2024

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
mcollina
mcollina reviewed Jul 4, 2024
View reviewed changes
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.

RedYetiDev
RedYetiDev reviewed Jul 4, 2024
View reviewed changes
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
RedYetiDev
RedYetiDev reviewed Jul 4, 2024
View reviewed changes
Copy link
Member

@RedYetiDev RedYetiDev left a comment •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60157/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60165/

@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
@anfibiacreativa @aduh95
http: expose websockets
ec0837a 
PR-URL: #53721
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit that referenced this pull request Jul 12, 2024
@aduh95
2024-07-15, Version 22.5.0 (Current)
d4d03e2 
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
Merged
aduh95 added a commit that referenced this pull request Jul 12, 2024
@aduh95
2024-07-15, Version 22.5.0 (Current)
df75c77 
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
@anfibiacreativa @aduh95
http: expose websockets
1367c55 
PR-URL: #53721
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit that referenced this pull request Jul 16, 2024
@aduh95
2024-07-16, Version 22.5.0 (Current)
b90d208 
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
@aduh95
2024-07-16, Version 22.5.0 (Current)
3f9c06e 
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
@aduh95
2024-07-16, Version 22.5.0 (Current)
433f759 
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
@aduh95
2024-07-17, Version 22.5.0 (Current)
34de839 
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
@aduh95 @RafaelGSS
2024-07-17, Version 22.5.0 (Current)
db1c2be 
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
@aduh95 @ehsankhfr
2024-07-17, Version 22.5.0 (Current)
274b746 
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

@RedYetiDev RedYetiDev mentioned this pull request Sep 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedYetiDev RedYetiDev RedYetiDev requested changes

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.