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
+60 −0

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.
RedYetiDev
RedYetiDev reviewed Jul 6, 2024
View reviewed changes
lib/http.js Outdated Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
RedYetiDev
RedYetiDev reviewed Jul 6, 2024
View reviewed changes
lib/http.js Outdated Show resolved Hide resolved
RedYetiDev
RedYetiDev reviewed Jul 7, 2024
View reviewed changes
lib/http.js Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
test/parallel/test-http-import-websocket.js Show resolved Hide resolved
anfibiacreativa and others added 5 commits July 8, 2024 13:08
@anfibiacreativa @RedYetiDev
http: remove additional export
0c2c82b 
Co-authored-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>
@anfibiacreativa @RedYetiDev
http: remove unnecessary declaration that was used in module.exports
aeed701 
Co-authored-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>
@anfibiacreativa @RedYetiDev
http: replace ternary
7209a89 
Co-authored-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>
@anfibiacreativa @RedYetiDev
http: rename function
c928a97 
Co-authored-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>
@anfibiacreativa
http: rename function invocation
cf3e9a7
@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 added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jul 8, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 8, 2024

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/9838611309
mcollina
mcollina approved these changes Jul 8, 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.

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