Skip to content

fix(e2ee): fix types#376

Merged
nbsp merged 6 commits intomainfrom
nbsp/fix/e2ee
Jan 7, 2025
Merged

fix(e2ee): fix types#376
nbsp merged 6 commits intomainfrom
nbsp/fix/e2ee

Conversation

@nbsp
Copy link

@nbsp nbsp commented Jan 6, 2025

No description provided.

@nbsp nbsp requested review from lukasIO and theomonnom January 6, 2025 20:51
@changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2025

🦋 Changeset detected

Latest commit: f9ac919

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

This PR includes changesets to release 6 packages
Name Type
@livekit/rtc-node Patch
@livekit/rtc-node-darwin-arm64 Patch
@livekit/rtc-node-darwin-x64 Patch
@livekit/rtc-node-linux-arm64-gnu Patch
@livekit/rtc-node-linux-x64-gnu Patch
@livekit/rtc-node-win32-x64-msvc 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

aoife cassidy added 2 commits January 7, 2025 01:40

this.options = options;
this.keyProvider = new KeyProvider(roomHandle, options.keyProviderOptions);
this.keyProvider =
Copy link
Contributor

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?

Copy link
Author

@nbsp nbsp Jan 7, 2025

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:

  1. assert its existence with !, which has broken things already
  2. 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

this.options = options;
this.keyProvider = new KeyProvider(roomHandle, options.keyProviderOptions);
this.keyProvider =
options.keyProviderOptions && new KeyProvider(roomHandle, options.keyProviderOptions);
Copy link
Contributor

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

Copy link
Author

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

aoife cassidy added 2 commits January 7, 2025 15:37
@lukasIO
Copy link
Contributor

lukasIO commented Jan 7, 2025

changeset is still missing

@nbsp nbsp merged commit aca0c42 into main Jan 7, 2025
13 checks passed
@nbsp nbsp deleted the nbsp/fix/e2ee branch January 7, 2025 16:32
@github-actions github-actions bot mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants