Skip to content

http: expose websocket in nodehttp#53721

Merged
nodejs-github-bot merged 27 commits into
nodejs:mainfrom
anfibiacreativa:feat/expose-websockets-in-nodehttp
Jul 8, 2024
Merged

http: expose websocket in nodehttp#53721
nodejs-github-bot merged 27 commits into
nodejs:mainfrom
anfibiacreativa:feat/expose-websockets-in-nodehttp

Conversation

@anfibiacreativa
Copy link
Copy Markdown
Member

@anfibiacreativa anfibiacreativa commented Jul 4, 2024

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
Copy Markdown
Collaborator

nodejs-github-bot commented Jul 4, 2024

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
Copy Markdown
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?

Comment thread lib/http.js Outdated
@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 4, 2024
@avivkeller
Copy link
Copy Markdown
Member

avivkeller commented Jul 4, 2024

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.

avivkeller
avivkeller reviewed Jul 4, 2024
View reviewed changes
Comment thread test/parallel/test-http-import-websocket.js Outdated
Comment thread lib/http.js
Comment thread lib/http.js
@avivkeller avivkeller self-requested a review July 4, 2024 21:21
avivkeller
avivkeller reviewed Jul 4, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@avivkeller avivkeller 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.

@avivkeller avivkeller 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
Copy Markdown

PuruVJ commented Jul 5, 2024

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