-
Notifications
You must be signed in to change notification settings - Fork 104
fix(e2ee): fix types #376
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(e2ee): fix types #376
Conversation
🦋 Changeset detectedLatest commit: f9ac919 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
packages/livekit-rtc/src/e2ee.ts
Outdated
|
|
||
| this.options = options; | ||
| this.keyProvider = new KeyProvider(roomHandle, options.keyProviderOptions); | ||
| this.keyProvider = |
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.
I'm not following the logic here, when and why would we accept keyProvider to be optional?
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.
i'm not sure it will be, but if we don't set it here, we have to either:
- assert its existence with
!, which has broken things already - not assume it exists, even though it definitely does
out of the two this is the less-likely-to-break-things option, since it completely bypasses the E2EEOptions | E2eeOptions issue
packages/livekit-rtc/src/e2ee.ts
Outdated
| this.options = options; | ||
| this.keyProvider = new KeyProvider(roomHandle, options.keyProviderOptions); | ||
| this.keyProvider = | ||
| options.keyProviderOptions && new KeyProvider(roomHandle, options.keyProviderOptions); |
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.
but this line here sets keyProvider to false/undefined if there are no keyProviderOptions passed. That doesn't seem right. There should always be a key provider, even if it's initialized with the default options
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 see. the default values were ignored outside of this function. latest commit runs the e2ee options through ...defaultE2EEOptions, before continuing in RoomOptions
|
changeset is still missing |
No description provided.