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

Use correct return type for RTCSession encryption keys #4423

Closed
wants to merge 1 commit into from

Conversation

hughns
Copy link
Member

@hughns hughns commented Sep 23, 2024

The functions have always possibly returned undefined where the key at a particular index was not known. This changes the return type to match this reality.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

This changes the return type of some public functions
@hughns hughns changed the title Returned MatrixRTC encryption key is undefined is not known Use correct return type for RTCSession encryption keys Sep 23, 2024
@hughns hughns marked this pull request as ready for review September 23, 2024 11:28
@hughns hughns requested a review from a team as a code owner September 23, 2024 11:28
hughns added a commit to element-hq/element-call that referenced this pull request Sep 23, 2024
As per matrix-org/matrix-js-sdk#4423 the key can be undefined and so we should handle this rather than waiting for SubtleCrypto.importKey() to fail.
@@ -483,6 +486,7 @@ export class MatrixRTCSession extends TypedEventEmitter<MatrixRTCSessionEvent, M
}
}

// n.b. this will extend the array if necessary and fill in the gaps with undefined
Copy link
Member

Choose a reason for hiding this comment

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

It will actually make it a sparse array with empty slots that are skipped by iteration methods.

Which gets me thinking, maybe callers can hook into that...

Copy link
Member

Choose a reason for hiding this comment

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

maybe callers can hook into that...

That is indeed the case; see element-hq/element-call#2646 (comment) which makes the updated typehints of this PR unnecessary (though the comments & updated unit test remain useful).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, TypeScript doesn't seem to model sparse arrays at all. So, as far as I can see, there is no compile time hint that a sparse array can contain empty values.

I don't find this very satisfactory. /me ponders

Copy link
Member

Choose a reason for hiding this comment

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

There is --noUncheckedIndexedAccess, but that would require updating many other parts of the codebase to comply with it.

@hughns
Copy link
Member Author

hughns commented Sep 23, 2024

I've made #4427 as an alternative which cleans up the API entirely. I prefer that as an approach to this.

@hughns hughns closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants