Add reconnecting websocket class and use it in CoderApi #654
Conversation
8529e15 to
5a28ca8
Compare
5a28ca8 to
f10c59b
Compare
f10c59b to
656bb4c
Compare
| // Don't reconnect on normal closure | ||
| if (event.code === 1000 || event.code === 1001) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: magic numbers are magic.. const these?
| if (watcher.error !== undefined) { | ||
| watcher.error = undefined; | ||
| onChange.fire(null); | ||
| } |
There was a problem hiding this comment.
What is the context behind this change?
There was a problem hiding this comment.
If we reach this place in the code that means we didn't encounter an error while parsing the message and thus we have a valid message so we clear the error (if any). The clients of this method usually display the error (if it exists) before attempting to access the value
| initialBackoffMs: options.initialBackoffMs ?? 250, | ||
| maxBackoffMs: options.maxBackoffMs ?? 30000, | ||
| jitterFactor: options.jitterFactor ?? 0.1, |
There was a problem hiding this comment.
How did we come up with these default values? This is non-blocking, just curious 😄
There was a problem hiding this comment.
Oh very random and some AI "researched" values hahaha
| wasClean: true, | ||
| type: "close", | ||
| target: this.#currentSocket, | ||
| } as CloseEvent); |
There was a problem hiding this comment.
Does this need as CloseEvent? Could we make use of satisfies?
There was a problem hiding this comment.
Thanks for pointing this out, this exposed a bigger issue with types, I made sure to narrow the types in eventStreamConnection so it's reflects the information we have and we don't do some nasty casting
* Better error handling for HTTP errors * Refactor magic numbers * Use strict types and avoid casting
DanielleMaywood
left a comment
There was a problem hiding this comment.
I'm happy with this change, thanks for addressing the types issue 🙏
Closes #595
This PR implements a reconnecting WebSocket with exponential backoff that automatically uses the most up-to-date authentication information.
Why not use existing packages?
Third-party reconnecting WebSocket packages (e.g., reconnecting-websocket) were considered but don't meet our requirements:
Additional features: