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

feat: add connect in spaceV2 #711

Merged
merged 7 commits into from
Sep 15, 2023
Merged

feat: add connect in spaceV2 #711

merged 7 commits into from
Sep 15, 2023

Conversation

arn4b
Copy link
Contributor

@arn4b arn4b commented Sep 15, 2023

No description provided.

@github-actions
Copy link

All looks good.

@github-actions
Copy link

All looks good.

@github-actions
Copy link

All looks good.

@github-actions
Copy link

  • In the SpaceV2 class constructor, the setSpaceV2Data function is being called twice. The first call is unnecessary and can be removed.
  • In the SpaceV2 class constructor, the this.setSpaceV2Data = function (fn) {...} expression is assigning a function to the setSpaceV2Data property. Instead, it should be assigned directly as this.setSpaceV2Data = fn;.
  • The connect function in connect.ts is missing type annotations for the options parameter. It should have the type IConnectOptions as specified in the function signature.
  • In the connect function, the peerConnection variable is being assigned the return value of this.getPeerConnection(...) and then immediately accessed with peerConnection?.on(...). It would be better to use optional chaining directly in the on method call like this.getPeerConnection(...)?.on(...).
  • In the connect function, the peerConnection?.on('error', ...) block does not handle rejected promises from the this.request function call. It should use try...catch to handle any errors that may occur within the try block.
  • In the connect function, the peerConnection?.signal(signalData) line is not important and should be removed, since peerConnection is already assigned a value in the previous line.
  • In the connect function, the this.setSpaceV2Data(...) block updates the status property of incomingPeerStreams based on the peerAddress. However, it should also update the retryCount property if the status changes to VideoCallStatus.CONNECTED.

@github-actions
Copy link

  • In the import section, it is better to separate the import statements with empty lines for better readability.

  • The SpaceV2 class has a member variable data declared without a type annotation. Add the type annotation for better clarity.

  • The setSpaceV2Data function in the SpaceV2 class is duplicated. Remove the duplicate initialization.

  • In the setSpaceV2Data function, update this.data after updating the react state.

  • In the constructor of the SpaceV2 class, assign setSpaceV2Data parameter directly to this.setSpaceV2Data.

  • In the addPeer function, return early if the peerId already exists, instead of logging an error message.

  • In the removePeer function, return early if the peerId does not exist, instead of logging an error message.

  • In the getPeerConnection function, the return type should be RTCPeerConnection | undefined, not just RTCPeerConnection.

  • In the setPeerConnection function, there is no need to explicitly set the type of peerConnection as RTCPeerConnection | undefined, since TypeScript can infer it automatically.

  • In the getConnectedPeerIds function, use the Array.from method directly on this.peerConnections.keys() instead of assigning it to a variable.

  • In the connect function, the peerConnection type should be RTCPeerConnection | undefined, not just any.

  • In the connect function, handle the case when peerConnection is undefined instead of calling methods on it directly.

@github-actions
Copy link

All looks good.

@github-actions
Copy link

  1. In the import section, there are unused imports for "immer" and "./join".
  2. In the import section, there is a typo in the import path "./inviteToJoin".
  3. In the import section, there is a missing import for "../helpers/pCAIP10ToWallet".
  4. The constant "ENV" imported from "../constants" is not used.
  5. The type "SignerType" imported from "../types" is not used.
  6. In the "initSpaceInfo" object, the "inviteeDetails" property should be assigned with an empty object, instead of an empty string.
  7. The "setSpaceV2Data" function in the "SpaceV2ConstructorType" interface is not correctly typed. It should accept a parameter of type "SpaceV2Data" and return "void".
export interface SpaceV2ConstructorType extends EnvOptionsType {
    signer: SignerType;
    pgpPrivateKey: string;
    chainId: number;
    address: string;
    setSpaceV2Data: (fn: (data: SpaceV2Data) => SpaceV2Data) => void;
}
  1. The "addPeer" method in the "SpaceV2" class is missing type annotations for the parameters. It should be:
addPeer(peerId: string, peerConnection: RTCPeerConnection | undefined) {
  1. The "removePeer" method in the "SpaceV2" class is missing type annotations for the parameters. It should be:
removePeer(peerId: string) {
  1. The "getPeerConnection" method in the "SpaceV2" class is missing type annotations for the parameters and return type. It should be:
getPeerConnection(peerId: string): RTCPeerConnection | undefined {
  1. The "setPeerConnection" method in the "SpaceV2" class is missing type annotations for the parameters. It should be:
setPeerConnection(peerId: string, peerConnection: RTCPeerConnection | undefined) {
  1. The "getConnectedPeerIds" method in the "SpaceV2" class is missing type annotations for the return type. It should be:
getConnectedPeerIds(): string[] {
  1. The "request", "invite", "connect", and "disconnect" methods in the "SpaceV2" class are missing type annotations for the parameters. They should be:
async connect(options: IConnectOptions) {
async disconnect(options: any) {
async request(options: any) {
async invite(options: ISpaceInviteInputOptions) {
  1. In the "connect" function, the type annotation for the "peerAddress" parameter is incorrect. It should be:
peerAddress: string | undefined
  1. The "connect" function should have a return type of "Promise"
export async function connect(
    this: SpaceV2,
    options: IConnectOptions
): Promise<void> {

All looks good.

@0xNilesh 0xNilesh merged commit 76dab91 into feat/space-v2 Sep 15, 2023
1 check passed
@0xNilesh 0xNilesh deleted the arnab/connect branch September 15, 2023 10:20
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