-
Notifications
You must be signed in to change notification settings - Fork 359
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
fix(calling): wdm deregister method #3609
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
packages/webex/src/calling.js
Outdated
const lineIds = Object.keys(this.callingClient?.getLines()); | ||
|
||
const lines = lineIds.length ? this.callingClient.getLines() : {}; | ||
for (const lineId of lineIds) { |
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.
Why can't we run the loop directly on the lines array? Why do we need to maintain another array of lineIds?
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.
- lines isn't an array. It is an object that contains
{ lineId: lineObject }
- When we do a for..in loop, it would go through prototype keys as well. Linter fails
- That is why we had to get lineIds in an array and then use it.
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.
getLines() API returns Record<string, ILine> so similar to how we are doing Object.keys() to fetch the keys which is lineId, we can do Object.values() which will return line array and then we can run for loop in the line array returned.
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.
Just one change needed regarding timeout
packages/webex/src/calling.js
Outdated
); | ||
resolve(); | ||
}); | ||
setTimeout(() => { |
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 won't need a timeout and reject here, once a deregister is called, assume that the user doesn't care about the result anymore.
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.
Ah. I agree. Makes sense. Let me remove it.
2521a9f
to
9b4add4
Compare
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.
Please take care of the last comment and check why integration test is failing
COMPLETES SPARK-509669
This pull request addresses
2 device registrations occur in the Calling SDK.
calling.register
which calls theinternal.device.register
fromwebex/calling.js
line.register
which does the Mobius line registrationWhile we have the above two methods for deregistering, we do not have a deregister method only for
line
and not forcalling
which does the WDM device deletion operationby making the following changes
webex/calling.js
- The Calling class such that, users can do this deregister upon successful deregistering of linecalling.deregister()
method alone which will deregister all the active lines, disconnect mercury and delete WDM device as wellRecording
In the below recordings, you can see that the WDM device and Mercury registration doesn't happen before this change and they happen in the after-change screen recording,
Before:
Device.Deregister.-.Before.mov
After:
calling.deregister cleans up active lines automatically and deletes the WDM registration
Screen.Recording.2024-05-22.at.3.01.46.PM.mov
calling.deregister just deletes the WDM registration
Device.Deregister.-.After.line.deregister.mov
Change Type
The following scenarios where tested
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.