- Notifications
You must be signed in to change notification settings - Fork 35
Add reconnecting websocket class and use it in CoderApi#654
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
EhabY commented Nov 17, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
f10c59b to 656bb4cCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // Don't reconnect on normal closure | ||
| if(event.code===1000||event.code===1001){ | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: magic numbers are magic.. const these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if(watcher.error!==undefined){ | ||
| watcher.error=undefined; | ||
| onChange.fire(null); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the context behind this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did we come up with these default values? This is non-blocking, just curious 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh very random and some AI "researched" values hahaha
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| wasClean: true, | ||
| type: "close", | ||
| target: this.#currentSocket, | ||
| }asCloseEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need as CloseEvent? Could we make use of satisfies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
DanielleMaywood left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this change, thanks for addressing the types issue 🙏
* Better error handling for HTTP errors * Refactor magic numbers * Use strict types and avoid casting
eec4c99 to 55b793bCompared5c3cc0 into coder:mainUh oh!
There was an error while loading. Please reload this page.
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: