[ECO-5260] feat: initial local proxy implementation #2

ttypic · 2025-04-09T12:26:36Z

Initial local proxy implementation

Core functionality includes:

Spying on HTTP Requests and Responses
- Inspect outgoing HTTP requests from the client.
- Inspect and modify incoming HTTP responses.
Spying on WebSocket Messages
- Observe outgoing WebSocket messages from the client.
- Inspect and modify incoming WebSocket messages.
Injecting WebSocket Messages
- Simulate server messages by injecting WebSocket frames into the client session.
Breaking WebSocket and HTTP Connections
- Force WebSocket disconnections to test reconnection behavior.
- Simulate slow or broken HTTP responses to test timeout handling.

Core functionality includes: - Spying on HTTP Requests and Responses - Inspect outgoing HTTP requests from the client. - Inspect and modify incoming HTTP responses. - Spying on WebSocket Messages - Observe outgoing WebSocket messages from the client. - Inspect and modify incoming WebSocket messages. - Injecting WebSocket Messages - Simulate server messages by injecting WebSocket frames into the client session. - Breaking WebSocket and HTTP Connections - Force WebSocket disconnections to test reconnection behavior. - Simulate slow or broken HTTP responses to test timeout handling.

VeskeR

Looks interesting!
Observing and injecting APIs by itself should enable replacing a significant amount of private API usage in LiveObjects tests, as these tests often either waited for a specific message to arrive over WebSocket or injected a bunch of messages to emulate server behavior.

I've added some comments in the code below, along with more general suggestions here:

Either use the explicit public modifier for public fields everywhere, or omit it everywhere. Currently, it's mixed - see public async start() and pauseAllConnections(): () => void.
Regarding naming for proxy public methods: I imagine that methods using the observe* naming convention would accept a callback to continuously notify the caller about events (i.e., actively observing something). However, this is not the case with the current public API, where observe* methods just return the next encountered request or message. Should we rename them to something more better suited, such as waitForNextRequest, waitForNextOutgoingProtocolMessage, or even dropping the waitFor prefix: e.g., proxy.nextRequest() or proxy.nextOutgoingProtocolMessage()?

VeskeR · 2025-04-14T12:03:23Z

test/utils.ts

+  await observerPromise;
+}
+
+export async function openConnection(ws: WebSocket) {


probably worth renaming to waitForConnectionOpen to keep parity with other methods below

VeskeR · 2025-04-14T12:09:51Z

src/completable-deferred.ts

+
+class DefaultCompletableDeferred<T = void> {
+  private _completeWith!: CompletionHandler<T>;
+  private value: T | undefined


it look like value is never set? is it still needed?

VeskeR · 2025-04-14T12:11:24Z

src/completable-deferred.ts

@@ -0,0 +1,22 @@
+type CompletionHandler<T = void> = (value: T) => void
+
+class DefaultCompletableDeferred<T = void> {


would like to have a description for this primitive as it's not immediately obvious what it does.
was it by any chance copied from somewhere? maybe can include a link to source too

VeskeR · 2025-04-14T12:19:18Z

src/async-queue.ts

+      try {
+        await task();
+      } finally {
+        deferredValue.complete();


we're not catching an error here and I believe it will lead to undandled promise rejections at the line below where we call this.processNext() (as that will throw an error when executin task).
is it intentional here?
also not handling an error prevents us from passing the error to the deferredValue - caller of the async-queue can't know if it failed

VeskeR · 2025-04-14T12:20:50Z

src/async-queue.ts

+    const task = this.queue.shift();
+
+    try {
+      await task!!();


Suggested change

await task!!();

await task!();

should it be just one exclamation mark?

VeskeR · 2025-04-14T12:26:03Z

README.md

+proxy.dropConnection(client.connection.id);
+
+// Pause and resume connection manually
+const resume = proxy.pauseConnection();


pauseConnection is not in the InterceptingProxy interface. should it be pauseAllConnections?
that being said we should still have a way to pause connection by an id. wdyt?

VeskeR · 2025-04-15T10:09:07Z

test/proxy.test.ts

+      async () => testWs.send(JSON.stringify({ action: 1 })),
+      async () => {
+        const message = await proxy.observeNextIncomingProtocolMessage();
+        const rawWebsocketMessage = await waitForNextReceivedMessage(testWs);


can these two message be different somehow? why do we need to check both?

VeskeR · 2025-04-15T10:39:19Z

src/proxy.ts

+  private readonly proxy = httpProxy.createProxyServer({});
+  private readonly wss = new WebSocketServer({ noServer: true });
+
+  private readonly server: http.Server = http.createServer((req: http.IncomingMessage, res: http.ServerResponse) => {


Might be worth extracting this createServer logic and similar https://github.com/ably-labs/local-proxy-js/pull/2/files#diff-78bb005ca752086400a648d8aa3e60ce6f666e213da508caabd464d3bfeaf6a2R235 and https://github.com/ably-labs/local-proxy-js/pull/2/files#diff-78bb005ca752086400a648d8aa3e60ce6f666e213da508caabd464d3bfeaf6a2R253 for websocket server to separate functions/classes with a bit more comments to explain what's going on in them.

As far I as I can see these are the parts that intercept http and ws traffic , so we can call them InterceptingHttpServer and InterceptingWebSocketServer

VeskeR · 2025-04-15T10:44:23Z

src/proxy.ts

+    const deferredValue = CompletableDeferred<Request<T>>();
+    const unregister = this.registerRestMiddleware(async (req, res) => {
+      if (!filter || filter(req)) deferredValue.complete(req);
+      return res;


Suggested change

return res;

return req;

shouldn't it return request?

ttypic requested a review from VeskeR April 9, 2025 12:26

ttypic force-pushed the ECO-5260/add-intial-proxy-impl branch 2 times, most recently from b21bea7 to 3323ede Compare April 10, 2025 12:25

ttypic requested a review from lawrence-forooghian April 10, 2025 12:44

ttypic force-pushed the ECO-5260/add-intial-proxy-impl branch 4 times, most recently from 3a2342b to 3c6051e Compare April 11, 2025 09:48

ttypic force-pushed the ECO-5260/add-intial-proxy-impl branch from 3c6051e to dd96ae9 Compare April 14, 2025 09:24

VeskeR requested changes Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ECO-5260] feat: initial local proxy implementation #2

[ECO-5260] feat: initial local proxy implementation #2

Uh oh!

ttypic commented Apr 9, 2025 •

edited

Loading

Uh oh!

VeskeR left a comment

Uh oh!

VeskeR Apr 14, 2025

Uh oh!

VeskeR Apr 14, 2025

Uh oh!

VeskeR Apr 14, 2025

Uh oh!

VeskeR Apr 14, 2025

Uh oh!

VeskeR Apr 14, 2025

Uh oh!

VeskeR Apr 14, 2025

Uh oh!

VeskeR Apr 15, 2025

Uh oh!

VeskeR Apr 15, 2025

Uh oh!

VeskeR Apr 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,22 @@
		type CompletionHandler<T = void> = (value: T) => void

		class DefaultCompletableDeferred<T = void> {

[ECO-5260] feat: initial local proxy implementation #2

Are you sure you want to change the base?

[ECO-5260] feat: initial local proxy implementation #2

Uh oh!

Conversation

ttypic commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VeskeR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ttypic commented Apr 9, 2025 •

edited

Loading