Repository navigation
feat(PLA-3932): connect sockets via WebSocket gateway - #1085
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bundle ReportChanges will increase total bundle size by 7.15kB (1.18%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: @knocklabs/client-client-cjsAssets Changed:
Files in
Files in
Files in
view changes for bundle: @knocklabs/client-client-esmAssets Changed:
Files in
Files in
Files in
|
699b940 to
6ecbbf2
Compare
| onConnectUrlInvalid: connectUrlCache.invalidate, | ||
| }); | ||
|
|
||
| this.socket = new Socket(`${this.host.replace("http", "ws")}/ws/v1`, { |
There was a problem hiding this comment.
The URL we’re passing to the Socket constructor here, ${this.host.replace("http", "ws")}/ws/v1, is overridden entirely by the signed URL we resolve in GatewayTransport.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved. This is a client SDK feature change, not a dependency or lock-file update, so it is outside this automation’s approval criteria. Human review is needed and no reviewers were assigned.
Sent by Cursor Approval Agent: Depdency updates (control, CLI, javascript, telegraph)
cjbell
left a comment
cjbell
left a comment
There was a problem hiding this comment.
This is great Matt. Super nice way to implement this. Very easy to follow code.
| export type TransportConstructor = new ( | ||
| url: string, | ||
| protocols?: string | string[], | ||
| ) => WebSocketLike; | ||
|
|
There was a problem hiding this comment.
Nit: would be nice if this was an object instead of positional function params in case of future expansion.
There was a problem hiding this comment.
Phoenix expects these two params, so we unfortunately can’t change this.
| * The signature that Phoenix expects socket transports to conform to, based on the native browser | ||
| * `WebSocket`. Only fields and methods that Phoenix relies on are included here. | ||
| */ | ||
| export interface WebSocketLike { |
There was a problem hiding this comment.
I think this can be derived from this type instead of manually declaring
Pick<WebSocket, "readyState" | "bufferedAmount" | "binaryType" | "onopen" | "onclose" | "onmessage" | "onerror" | "send" | "close">
| export async function fetchSocketConnectUrl( | ||
| apiClient: ApiClient, | ||
| signal: AbortSignal, | ||
| ): Promise<GatewayConnectResult> { |
There was a problem hiding this comment.
same thing about function parmas here too, might just be worth doing a pass on this.
There was a problem hiding this comment.
This one can in fact change; it’s not tied to Phoenix.
| const connectUrlCache = cacheUntilExpired((signal: AbortSignal) => | ||
| fetchSocketConnectUrl(this, signal), | ||
| ); |
There was a problem hiding this comment.
we don't actually handle the failure here when this function throws fetchSocketConnectUrl
There was a problem hiding this comment.
If this fetch rejects, the error is not handled here, but in the gateway transport. fetchSocketConnectUrl is called whenever connectUrlCache.get() is called, and connectUrlCache.get is passed to the gateway transport as fetchConnectUrl. Any rejection from fetchConnectUrl is handled in GatewayTransport’s start method:
javascript/packages/client/src/gatewayTransport.ts
Lines 178 to 188 in 6ecbbf2
I’ll add a comment to clarify this.
| onmessage: ((event: MessageEvent) => void) | null = null; | ||
| onerror: ((event: unknown) => void) | null = null; | ||
|
|
||
| private inner: WebSocketLike | null = null; |
There was a problem hiding this comment.
should this be named self instead of inner?
There was a problem hiding this comment.
The inner field holds the wrapped WebSocket created by the initBaseTransport method. It’s a separate transport that GatewayTransport delegates to, but it’s not the GatewayTransport itself. this.self.send(data) reads like the gateway transport is calling itself rather than delegating.
| // The base transport's constructor threw an exception. We never even attempted a | ||
| // connection, so onclose won't fire. The connect URL must be invalid. | ||
| onConnectUrlInvalid?.(); | ||
| this.fail(error); |
There was a problem hiding this comment.
same here, should we just be returning errors to get them at callsite if needed instead of returning void?
There was a problem hiding this comment.
See above; we need to emit error events just like WebSocket.
| const inner = new baseTransport(url, this.protocols); | ||
|
|
||
| inner.binaryType = this.binaryType; | ||
| inner.onopen = (ev) => { |
There was a problem hiding this comment.
It’s the event that’s triggered when the WebSocket connection opens. These onopen, onmessage, onerror, and onclose handlers forward every event to the event handlers registered by Phoenix’s Socket so that Phoenix can manage socket state.
| export function getQueryString(url: string): string { | ||
| const idx = url.indexOf("?"); | ||
| return idx === -1 ? "" : url.slice(idx + 1); | ||
| } |
There was a problem hiding this comment.
should use built in URL class to make this cleaner and less error prone.
There was a problem hiding this comment.
React Native has some known issues related to URL and URLSearchParams. See react/react-native#38656 and the README for react-native-url-polyfill. It appears that these issues were fixed in React Native v0.84, but our React Native SDK currently supports back to v0.81:
I’ll add a comment to explain this.
| export function appendQueryString(url: string, queryString: string): string { | ||
| return queryString | ||
| ? `${url}${url.includes("?") ? "&" : "?"}${queryString}` | ||
| : url; | ||
| } |
There was a problem hiding this comment.
See above. There are known issues with React Native’s implementation of URL.
| export type CachedFetch<Args extends unknown[], T> = { | ||
| /** | ||
| * Returns the cached value if still valid, otherwise calls `fetchFunc` with the same args and | ||
| * caches the result. | ||
| */ | ||
| get: (...args: Args) => Promise<T>; | ||
| /** | ||
| * Drops any cached value so the next `get` call fetches again. | ||
| */ | ||
| invalidate: () => void; | ||
| }; | ||
|
|
||
| export const CACHE_EXPIRY_PADDING_MS = 5_000; | ||
|
|
||
| /** | ||
| * Wraps `fetchFunc` so its result is cached until `expiresAt` (epoch ms), minus | ||
| * `CACHE_EXPIRY_PADDING_MS`. | ||
| */ | ||
| export function cacheUntilExpired< | ||
| Args extends unknown[], | ||
| T extends { expiresAt: number }, | ||
| >(fetchFunc: (...args: Args) => Promise<T>): CachedFetch<Args, T> { | ||
| let cached: T | null = null; | ||
|
|
||
| const get = async (...args: Args) => { | ||
| if (cached && cached.expiresAt - CACHE_EXPIRY_PADDING_MS > Date.now()) { | ||
| return cached; | ||
| } | ||
| const value = await fetchFunc(...args); | ||
| cached = value; | ||
| return value; | ||
| }; | ||
|
|
||
| const invalidate = () => { | ||
| cached = null; | ||
| }; | ||
|
|
||
| return { get, invalidate }; | ||
| } |
There was a problem hiding this comment.
should use tanstack store for this stuff, it's already a dep.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mattmik-kno-16047-sdks-add-maxretries-option-to-apirequestconfig #1085 +/- ##
====================================================================================================
+ Coverage 75.42% 76.04% +0.61%
====================================================================================================
Files 139 140 +1
Lines 4379 4495 +116
Branches 1288 1310 +22
====================================================================================================
+ Hits 3303 3418 +115
- Misses 976 977 +1
Partials 100 100
|
Description
This PR updates the client SDK’s
ApiClientto establish a WebSocket connection using the WebSocket gateway API endpoint. Now, before a WebSocket connection is attempted, we’ll make a request to the/v1/sockets/connectAPI endpoint, which returns a signedwss://URL and an expiration time. We then establish a WebSocket connection using this signed URL, rather than thewss://api.knock.app/ws/v1/websocketendpoint which was previously hardcoded.Phoenix’s
Socketclass supports specifying a customtransport. Normally this would be set to something likeWebSocketorLongPoll, but any custom transport can be specified as long as it obeys theWebSocketcontract. For example, instances of the transport should have areadyStateproperty, asendmethod, aclosemethod, etc.So, this PR adds a custom transport
GatewayTransport, constructed via the factory functioncreateGatewayTransport.GatewayTransportis configured using afetchConnectUrloption, a function used to make a request to the/v1/sockets/connectendpoint via the API client.As soon as a
GatewayTransportis instantiated, we callfetchConnectUrlto make the API request. Until this request resolves, the transport’sreadyStateremainsCONNECTING. Assuming the request resolves successfully,GatewayTransportthen delegates to a “base” transport (which will always beWebSocket) to establish a connection using the signed URL that was resolved viafetchConnectUrl. From that point forward,GatewayTransport’s properties and methods will always delegate to the instance of the base transport that was created.A
cacheUntilExpiredhelper function is used to skip unnecessary calls to the WebSocket gateway API endpoint; while a signed URL has not expired, any socket reconnection initiated by Phoenix will use that same signed URL. Once the URL expires, any reconnection will create an entirely new instance ofGatewayTransportand callfetchConnectUrlto retrieve a new signed URL via the gateway endpoint.Note that whenever
fetchConnectUrlis called to make a request to the gateway endpoint, we set themaxRetriesoption to 0 so thatApiClient’s own retry logic is bypassed. This is because Phoenix’sSocketalready retries using exponential backoff, as defined using thereconnectAfterMsoption.Checklist
Screenshots or videos
Loom demo and code walkthrough