Typesafe error propagation in signal connection path #1747
Conversation
This reverts commit d920fa7.
🦋 Changeset detectedLatest commit: 90d9eca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
Updated version type for livekit-client from patch to minor.
…dk-js into lukas/neverthrow-signal
| export class WebSocketStream<T extends ArrayBuffer | string = ArrayBuffer | string> { | ||
| readonly url: string; | ||
|
|
||
| readonly opened: Promise<WebSocketConnection<T>>; | ||
| readonly opened: ResultAsync<WebSocketConnection<T>, WebSocketError>; | ||
|
|
||
| readonly closed: Promise<WebSocketCloseInfo>; | ||
| readonly closed: ResultAsync<WebSocketCloseInfo, WebSocketError>; | ||
|
|
||
| readonly close: (closeInfo?: WebSocketCloseInfo) => void; | ||
| readonly close!: (closeInfo?: WebSocketCloseInfo) => void; |
There was a problem hiding this comment.
suggestion: if the plan is to eventually migrate to the in-built browser WebsocketStream (and from what I can see, it is on some sort of standards track), it might be worth treating this like vendored code that shouldn't be modified and doing these ResultAsync patches where this is being used in the signal client code.
I can think of pros and cons to both approaches though so I'd be curious to hear your rationale if there is a reason why doing this would be overly burdensome.
There was a problem hiding this comment.
Fair point. Was thinking about this too. I have currently less confidence into adapting the standard proposal any time soon. Given that, I think starting with the error propagation at the lowest level sounded like the right choice to me
There was a problem hiding this comment.
some minor comments.
From the general code structure perspective, I think it is the right direction to setup a pattern where the code will throw.
I think moving the throw code to upper layer sounds a good idea, assuming the lower level code stop throwing for expected errors.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Looks good to me! Just generally, I think I'm realizing there's some fairly nuanced behavior with neverthrow that doesn't carry over 1:1 with how rust handles errors, so I think I'll need to spend some time digging into some of those nuances.
| const self = this; | ||
|
|
||
| const handleSignalConnected = ( | ||
| connection: WebSocketConnection, | ||
| firstMessage?: SignalResponse, | ||
| ) => { | ||
| this.handleSignalConnected(connection, wsTimeout, firstMessage); | ||
| }; | ||
| return withMutex( | ||
| safeTry<U, ConnectionError>(async function* () { |
There was a problem hiding this comment.
suggestion(non-blocking): When I was first trying to parse through this I saw you swapped all this -> self and it took me a second to figure out exactly where self was being assigned and why you had done this. It looks like (from what I can tell) it's due to the generator function creating a new explicit this scope.
I wanted to propose another option instead - something like async function* () {}.bind(this). There's some discussion here on the pros/cons: supermacro/neverthrow#632. At least as somebody coming into reading this code for the first time, I think it would help but it's minor and something I don't have a strong opinion on.
There was a problem hiding this comment.
Generally 50:50 on both, but reading through the issue you linked it mentions two steps being necessary for typescript today to properly bind it, i.e.: return safeTry(function* (this: CurrentClass) {}).bind(this) which would sway me to leave it as is.
| reason: unknown, | ||
| validateUrl: string, | ||
| ): Promise<ConnectionError> { | ||
| ): Promise<Result<never, ConnectionError>> { |
There was a problem hiding this comment.
nitpick: I think this should be ResultAsync instead and the returns from this function updated to be errAsync?
UPDATE: reading a little further, this might actually be intended. I'm still leaving this comment though because I think it's relevant to a future comment.
| return ResultAsync.fromPromise(Promise.race(settledPromises), (e) => e as E); | ||
| } | ||
|
|
||
| export type ResultAsyncLike<T, E> = ResultAsync<T, E> | Promise<Result<T, E>>; |
There was a problem hiding this comment.
question: Interesting - in my last comment I had assumed that Promise<Result<T, E>> was a mistake, but this line implies this is very much intentional. Is this a pattern because of async functions always returning promises, ie, the same type of topic as this issue?
Just thinking out loud but I wonder if handling this difference at this level of abstraction is the right place to do it (naively as somebody who hasn't really dug too much into neverthrow in depth yet, this semantic difference is surprising), or if it would be better to write a utility function which can convert any Promise<Result<T, E>> to a ResultAsync<T, E>.
I think I'd need to play around with it a bit on my own to develop a stronger opinion so feel free to keep it how it is for now and we can discuss it in the future once I've been able to dig in further.
There was a problem hiding this comment.
yeah, it's on purpose but mostly as an intermediate step until we have ported all async internals to the result type.
The helper function is a nice idea, we'd then have to have it include a generic Error though as the Promise could still through and would have to be unwrapped safely.
There was a problem hiding this comment.
Is this a pattern because of async functions always returning promises, ie, the same type of topic as supermacro/neverthrow#42?
yes, this was the reason and for the helpers I thought having a helper type to accept both would be nice
| const self = this; | ||
| const restartResultAsync = safeTry(async function* () { |
There was a problem hiding this comment.
suggestion(non-blocking): another instance of https://github.com/livekit/client-sdk-js/pull/1747/files#r2546082926 - consider maybe also using .bind(this) here.
No description provided.