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.

Sign up for GitHub

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

Jump to bottom

Add missing createSIPParticipant params #337

Merged
merged 5 commits into from
Nov 28, 2024
Merged

Add missing createSIPParticipant params #337

merged 5 commits into from
Nov 28, 2024

Conversation

umarniz
Copy link
Contributor

@umarniz umarniz commented Nov 20, 2024

The following params exist in the Go SDK but not in the node SDK:

- RingingTimeout
- MaxCallDuration
- EnableKrisp

They were added last month to the protocol: livekit/protocol@19b686d

Tested in my local project to work correctly.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 20, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 45a9390

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
livekit-server-sdk Minor
agent-dispatch Patch

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

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@umarniz
Copy link
Contributor Author

umarniz commented Nov 20, 2024

Added a change set as I assume there will be a minor version bump with this change. Feel free to take over the PR as a maintainer or ping me if I can help with something.

@umarniz
Copy link
Contributor Author

umarniz commented Nov 24, 2024

Pinging @dennwc for review as I saw you reviewed another SIP PR.

Hope not the wrong reviewer 🙏🏽

davidzhao
davidzhao reviewed Nov 28, 2024
View reviewed changes
packages/livekit-server-sdk/src/SipClient.ts Outdated
Comment on lines 454 to 455
ringingTimeout: new Duration({ seconds: BigInt(ringingTimeout) }),
maxCallDuration: new Duration({ seconds: BigInt(maxCallDuration) }),
Copy link
Member

Choose a reason for hiding this comment

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

when it's not passed in, we should leave these as undefined instead of set to 0. this way the server is able to pick sane defaults, versus having different defaults from each client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidzhao thanks for the review, defaulted the 3 new params to be undefined if not specified

davidzhao
davidzhao approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

thanks for the PR!

@davidzhao davidzhao merged commit 1e372ed into livekit:main Nov 28, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Nov 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dennwc dennwc dennwc approved these changes

@davidzhao davidzhao davidzhao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@umarniz @CLAassistant @davidzhao @dennwc