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

[Node Compat] http.ClientRequest does not emit upgrade event (or is buggy) #17847

Closed
d4h0 opened this issue Dec 26, 2022 · 5 comments · Fixed by #19412
Closed

[Node Compat] http.ClientRequest does not emit upgrade event (or is buggy) #17847

d4h0 opened this issue Dec 26, 2022 · 5 comments · Fixed by #19412

Comments

@d4h0
Copy link

d4h0 commented Dec 26, 2022

Hi,

I tried to figure out how to solve #16899, and discovered that the HTTP client of the Node Compat layer does not emit the upgrade event.

This means, that no NPM package that contains a WebSocket client will work properly.

I tried really hard to figure out what's going on, and how to fix it, but I'm not really a Typescript programmer, and the code is hard to follow for me.

It seems, the HTTP client just does not implement the event (it is implemented for the server, however).

The Event: 'upgrade' example in the docs of the Node.js http module is a good way to reproduce the issue.

You can run it, as follows:

  1. Save the Event: 'upgrade' example in a file
  2. Change the import statement at the top to import * as http from "https://deno.land/[email protected]/node/http.ts";

If you now execute the script, it will just hang. With Node.js, it prints the expected text to the terminal.

Also (because it took me bit to figure this out), here is the easiest way to debug the issue, I've found:

  1. Clone the standard lib: git clone https://github.com/denoland/deno_std
  2. Change the import statement at the top of the script to import * as http from '/$PATH_TO/deno_std/node/http.ts'
  3. Now you can alter the stdlib code
  4. The http module is at node/http.ts
@lino-levan
Copy link
Contributor

This should probably be transferred over to denoland/deno_std(?)

@kt3k
Copy link
Member

kt3k commented Dec 28, 2022

Currently we use fetch internally in http client implementation of std/node (ref: https://github.com/denoland/deno_std/blob/75bd5657d60737951436b47f1626b2ec8a654baa/node/http.ts#L137)

So we don't have TcpConn object for that http request. That means even if we implemented 'upgrade' event, it doesn't enable websocket capability on it.

I think we probably need to sniff the option object of http.request API and switch it to use WebSocket instead of fetch if the request is trying to upgrade to websocket.

@Jakob5358
Copy link

I hope this will be done before 2.0

@d4h0
Copy link
Author

d4h0 commented Jun 11, 2023

@Jakob5358: It looks like, there will be an attempt to fix the issue soon: #18836

@bartlomieju
Copy link
Member

Implementation is almost done in #19412

bartlomieju added a commit that referenced this issue Jun 13, 2023
This commit adds support for "upgrade" events in "node:http"
"ClientRequest". Currently only "Websocket" upgrades are
handled. Thanks to this change package like "npm:puppeteer"
and "npm:discord" should work.

Closes #18913
Closes #17847
bartlomieju added a commit that referenced this issue Jun 15, 2023
This commit adds support for "upgrade" events in "node:http"
"ClientRequest". Currently only "Websocket" upgrades are
handled. Thanks to this change package like "npm:puppeteer"
and "npm:discord" should work.

Closes #18913
Closes #17847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants