Skip to content
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

Handle when rtcIceCandidateStatsReport is undefined #280

Conversation

phi-line
Copy link

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

Description

In the case where rtcIceCandidateStatsReport is never set, we cannot read from one of its properties. This change ensures that a default value is set when rtcIceCandidateStatsReport is undefined.

Burndown

Before review

  • Updated CHANGELOG.md if necessary
  • Added unit tests if necessary
  • Updated affected documentation
  • Verified locally with npm test
  • Manually sanity tested running locally
  • Ready for review

Before merge

  • Got one or more +1s
  • Squashed erroneous commits if necessary
  • Re-tested if necessary

@phi-line phi-line force-pushed the phi-line/undefined-rtcIceCandidateStatsReport branch from c0da4aa to a6ca00c Compare July 17, 2024 21:46
In the case where rtcIceCandidateStatsReport is never set, we cannot read from one of its
properties. This change ensures that a default value is set when rtcIceCandidateStatsReport
is undefined.
@phi-line phi-line force-pushed the phi-line/undefined-rtcIceCandidateStatsReport branch from a6ca00c to 291b8a1 Compare July 17, 2024 21:46
@@ -292,7 +292,7 @@ export class PreflightTest extends EventEmitter {
const report: PreflightTest.Report = {
callSid: this._callSid,
edge: this._edge,
iceCandidateStats: this._rtcIceCandidateStatsReport.iceCandidateStats,
iceCandidateStats: this._rtcIceCandidateStatsReport?.iceCandidateStats ?? [],
Copy link
Author

@phi-line phi-line Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iceCandidateStats cannot be undefined so we change to empty array.

export interface Report {
...
    iceCandidateStats: RTCIceCandidateStats[];
...

@charliesantos
Copy link
Collaborator

@phi-line thanks for submitting this PR. It's simple and I understand why this is needed. We'll take a closer look and make sure there are no side effects.

@phi-line
Copy link
Author

@charliesantos thanks for the reply. Just to add some more context:

I found an active issue with the sdk when running the pre-flight test. Looks like this property iceCandidateStats cannot be read when this._rtcIceCandidateStatsReport is undefined:

    const report: PreflightTest.Report = {
      callSid: this._callSid,
      edge: this._edge,
>>>   iceCandidateStats: this._rtcIceCandidateStatsReport.iceCandidateStats,
      networkTiming: this._networkTiming,
      samples: this._samples,
      selectedEdge: this._options.edge,
      stats,
      testTiming,
Cannot read properties of undefined (reading 'iceCandidateStats')

Seems like this needs better typescript handling so I created this PR that improves the handling when _rtcIceCandidateStatsReport is undefined

@charliesantos
Copy link
Collaborator

Thanks @phi-line . Seems like a good fix. We're currently working on this and expect a release in the next few days.

@charliesantos
Copy link
Collaborator

@phi-line I added some tests and made this PR #282
It will be included in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants