-
Notifications
You must be signed in to change notification settings - Fork 130
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
Register engine events on localParticipant when updated #480
Conversation
🦋 Changeset detectedLatest commit: cba4e6a 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 |
src/room/Room.ts
Outdated
@@ -233,15 +233,17 @@ class Room extends (EventEmitter as new () => TypedEmitter<RoomEventCallbacks>) | |||
} | |||
if (this.state === ConnectionState.Reconnecting) { | |||
log.info('Reconnection attempt replaced by new connection attempt'); | |||
// make sure we close and recreate the existing engine in order to get rid of any potentially ongoing reconnection attempts | |||
this.recreateEngine(); |
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.
should this fall through? It is calling createEngine
below through a connectFn
promise it looks like.
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.
guess that is okay as createEngine
checks for this.engine
?
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.
yeah, the wording on createEngine
isn't ideal as it won't actually do anything if the engine already exists.
Maybe renaming createEngine
to something like ensureEngine
would be the way to go?
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.
we use the name like maybeCreateEngine
on server side for such uses. ensureEngine
also works. Actually, createEngine
works as well. Just that the code segment was a bit confusing. It calls recreateEngine
. A few lines down, there is createEngine
(although that is actually inside a function variable which is used way down the function, I actually kept expanding that function in the review to see how far down that gets :-) )
* Register engine events on localParticipant when updated * changeset * add comment * remove debug code * remove log * remove log * rename createEngine, make codepath easier to follow * set connection state as early as possible
* Register engine events on localParticipant when updated * changeset * add comment * remove debug code * remove log * remove log * rename createEngine, make codepath easier to follow * set connection state as early as possible
* Register engine events on localParticipant when updated * changeset * add comment * remove debug code * remove log * remove log * rename createEngine, make codepath easier to follow * set connection state as early as possible
We didn't register the events after setting
localParticipant.engine
previously.Error surfaced when calling
recreateEngine
instead ofcreateEngine
during connect, where the localparticipant wouldn't notice that the engine had changed.