Effect-TS / platform

Unified interfaces for common platform-specific services
https://effect.website
MIT License
39 stars 12 forks source link

Node HttpClient treat just upgrade response as successful response #375

Closed leonitousconforti closed 8 months ago

leonitousconforti commented 9 months ago

I didn't make a reproducible example for this but I have a request the responds immediately with an "connection: upgrade" response and never sends anything in the body - it just upgrades the connection right away and then waits for your to send more data over the tcp socket. This results in the NodeCient request hanging forever because the server never responds with any data, just an upgrade request.

I have modified the waitForResponse handler so that it resumes if the server sends a response or a "connection: upgrade" (Because the node.js event emitter does not emit a response event for a "connection: upgrade" response)

Let me know if you think this is an adequate way to handle this case, or if you think of something else, or if you think the NodeClient shouldn't handle this at all and I should maybe wait for the websockets pr to see if that implements something I could use.

changeset-bot[bot] commented 9 months ago

🦋 Changeset detected

Latest commit: cb4338509ca68d0fd35e3829b70d9f992d87f1bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | --------------------- | ----- | | @effect/platform-node | Patch | | @effect/platform-bun | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

leonitousconforti commented 9 months ago

Hey @tim-smart, I see that all the repositories have been consolidated into a monorepo at https://github.com/Effect-TS/effect. Let me know if that repository is ready, and, if so should I close this and open a new PR there or if thats just generating too much noise right now? Thanks!

tim-smart commented 8 months ago

Feel free to move the PR over :)