[STREAM-1179] Allow a streaming-client instance to be marked as alertable#338
[STREAM-1179] Allow a streaming-client instance to be marked as alertable#338
Conversation
hjon
requested review from
maxwellmooney13,
patrickfeeney03 and
zservies
as code owners
hjon
force-pushed
the
STREAM-1179
branch
4 times, most recently
from
0229a40 to
be45ecb
Compare
…workaround a race condition
src/types/interfaces.ts
Outdated
| [header: string]: string; | ||
| } | ||
|
|
||
| export interface IAlertableInteractions extends Object { |
There was a problem hiding this comment.
We shouldn't need to extend Object, it should be implicitly extended already.
There was a problem hiding this comment.
Should this be a regular object or an array? I'm not sure I have a particularly strong opinion, but it did initially seem strange for this to be an object.
I figured we'd do this somewhat like we do allowedSessionTypes in the SDK - so it'd be something like ['voice', 'digital'] in the future. I suppose this lets us get even more specific within the space of digital but even then we could always make this an array of objects.
There was a problem hiding this comment.
I'll think about that. I don't quite remember why I landed here, but the reasons may no longer be relevant.
There was a problem hiding this comment.
I took another look and I think I agree with you. I suspect I had a certain idea of how this would be used that didn't pan out.
(I think Max made a similar suggestion early on, so between the two of you I came around. 🙂)
src/types/interfaces.ts
Outdated
| } | ||
|
|
||
| export interface IAlertableInteractions extends Object { | ||
| voice?: boolean; |
There was a problem hiding this comment.
In the future will we need to add video, digital, etc?
There was a problem hiding this comment.
Yep. So to your earlier comment, an array might be just as good.
| // STREAM-1204 | ||
| // There's a race condition between the backend service knowing about the connection | ||
| // and us marking the connection as alertable. For now, we'll just retry with some delay. | ||
| const maxRetries = 16; |
There was a problem hiding this comment.
My head says this is a big number but its only eight seconds of retries.
There was a problem hiding this comment.
Yeah...of course, "only" eight seconds is rather long...but I'm just trying to avoid any issues until we can improve it (I hope).
|
|
||
| if (this.config.jid) { | ||
| jidPromise = Promise.resolve(this.config.jid); | ||
| if (this.config.userId && this.config.jid) { |
There was a problem hiding this comment.
Should this be an &&? The userId field is optional it looks.
There was a problem hiding this comment.
Both userId and jid are optional. But we need the userId for alerting leader, so I didn't want to resolve the promise from the config if we didn't have both.
4 participants
No description provided.