SSE Edge #4119
SSE Edge #4119
Conversation
|
Build for latest commit 804d4cb is at https://pr4119.build.csb.dev/s/new. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 804d4cb:
|
|
This is ready to be merged, I tested it on staging, and the starter is already deployed in |
CompuIves
left a comment
CompuIves
left a comment
There was a problem hiding this comment.
Great, looks good! I left two questions that don't need to be addressed, but good to keep in mind.
| autoConnect: false, | ||
| transports: ['websocket', 'polling'], | ||
| query: { | ||
| sandboxid: this.sandboxId, |
There was a problem hiding this comment.
Do we lose any data in the manager with this? Might there be a reason in the future that we want to infer the sandbox id from the WS connection? If that's the case we might want to keep it in the URL, but I'm not sure if you already have this data.
There was a problem hiding this comment.
The client already sends the sandbox id to sse-manager in the websocket protocol.
| const usedHost = this.host || 'https://codesandbox.io'; | ||
| const sseHost = usedHost.replace('https://', 'https://sse.'); | ||
| const sseLbHost = usedHost.replace('https://', 'https://sse-lb.'); | ||
| const res = await axios.get(`${sseLbHost}/api/cluster/${this.sandboxId}`); |
There was a problem hiding this comment.
Maybe we should put this in a function that retries up to 3 times, in case of flaky connections or when the LB has an error.
There was a problem hiding this comment.
That's a good point, do we use any retry library anywhere that we could reuse here? Also I think some error handling would be nice, in case all retries fail, but I'm not sure what the UI/UX pattern for this is.
There was a problem hiding this comment.
We don't use anything generic right now, I have written a poor mans retry function. Would be good to introduce some common helper function in @codesandbox/common or a library I think.
Regarding the notice, we can send a notification from here. You can import
import {
notificationState,
NotificationStatus
} from '@codesandbox/common/lib/utils/notifications';
and do
notificationState.addNotification({ message: 'blah', status: NotificationStatus.Error })
Before SSE Edge™ we had a single cluster, a single
sse-manager, hence a single SSE websocket URL. With SSE Edge we have multiple of those, so for any one sandbox we first have to query the load-balancer for the cluster it's scheduled on, and from that derive the websocket URL.This was partially solved in #4228, by adding the sandbox id to the websocket query, so that the CF proxy can read it and query the load-balancer. This PR fixes it completely, by moving that logic to the client, so that it can connect the websocket directly, without going through the CF proxy, which will only be used for previews.