Skip to content

Remove http-proxy-node16, replace with lightweight inline proxy#7196

Closed
ryancbahan wants to merge 1 commit into
04-03-04-03-remove-network-interfacesfrom
04-03-04-03-remove-http-proxy
Closed

Remove http-proxy-node16, replace with lightweight inline proxy#7196
ryancbahan wants to merge 1 commit into
04-03-04-03-remove-network-interfacesfrom
04-03-04-03-remove-http-proxy

Conversation

@ryancbahan

@ryancbahan ryancbahan commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add http-proxy.ts (~100 lines) implementing web() and ws() proxy methods using Node's built-in http/https modules
  • HTTP forwarding: pipes request to target, pipes response back with headers
  • WebSocket forwarding: handles upgrade handshake, bidirectional socket piping, proper error cleanup
  • Remove http-proxy-node16 from packages/app/package.json

Tophatting

  • confirm shopify app dev still works as expected
  • confirm dev console in admin still receives ws updates from cli

ryancbahan commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

Add http-proxy.ts (~100 lines) implementing web() and ws() proxy
methods using Node's built-in http/https modules. Handles HTTP request
forwarding via pipe, and WebSocket upgrades via bidirectional socket
piping with proper 101 Switching Protocols response.

Includes 6 direct unit tests (GET/POST forwarding, header preservation,
path forwarding, error callbacks for unreachable targets on both web
and ws). Plus 8 existing integration tests via http-reverse-proxy.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ryancbahan ryancbahan force-pushed the 04-03-04-03-remove-http-proxy branch from 7008daf to 383b392 Compare April 3, 2026 23:02
@ryancbahan ryancbahan marked this pull request as ready for review April 3, 2026 23:09
@ryancbahan ryancbahan requested a review from a team as a code owner April 3, 2026 23:09
@github-actions

github-actions Bot commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

@gonzaloriestra gonzaloriestra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change. I agree with replacing simple libraries with our own code, but this looks a bit complex (even if it's small)...

For example, some concerns by Claude that were apparently handled by the library:

  1. connection: 'close' on every proxied request — this disables keep-alive, which could add latency for repeated requests to the same backend during app dev. A battle-tested proxy library would handle connection pooling properly.
  2. Manual HTTP response construction in WS upgrade — writing raw HTTP/1.1 101 Switching Protocols\r\n... strings is fragile. Edge cases around header formatting (multi-value headers, encoding) can cause subtle breakage.
  3. No timeout handling — if a backend hangs, the proxy request will hang indefinitely.

Copy link
Copy Markdown
Contributor Author

@gonzaloriestra I hear you, but I think a simple proxy/reverse proxy is something node can do well and that we can manage. But I definitely want to ask for broader team perspective here. @byrichardpowell @dmerand @isaacroldan what do y'all think? If there's alignment that this is a good dep removal candidate, I'll harden the tests and ensure those cases you pointed out are derisked. Else, I'll close.

Copy link
Copy Markdown
Contributor

Yeah, this one is not a clear candidate for removal imo. It seems like http-proxy-node16 hasn't had many updates in the last 24 months, so it's not really a maintenance sink. I also echo Gonzalo's concern about edge cases. Although I haven't looked into the practical implications of Gonzalo's points my instinct is to leave this one alone.

@ryancbahan ryancbahan closed this Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants