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

Clean up room connection status #102

Merged

Conversation

nandito
Copy link
Collaborator

@nandito nandito commented Oct 24, 2023

Summary

Clean up SDK's api surface before the beta release.

Changes

  • Remove accepted state from roomConnectionStatus
    > This state is not used for anything, it should be safe to remove.
  • Rename rejected state to knock_rejected
    > Use a more descriptive status name when a knock request gets rejected on a locked room.
  • Rename roomConnectionStatus -> connectionStatus
  • Remove mostRecentChatMessage
  • Remove startScreenshareError
  • Replace isStartingScreenshare with localScreenshareStatus and fix the missing state reset when screen share stops
  • Make roomConnectionOptions optional
    > Use default localMediaConstraints' audio, video: true.
  • Use the default media constraints in the related story
    > useLocalMedia already has a default option and the same value was
    passed in the story. This commit removes this unnecessary param.
  • Do not expose newJoiner and streams on remoteParticipant
    > These are internal props, no need to expose them to in the useRoomConnection's state.

Test plan

knock:

  1. Use the sdk with a locked room
  2. Join the room with a host key
  3. Knock the room with a guest participant
  4. Reject the knock with the host => room connection status should be knock_rejected

screenshare status:

  1. start/stop screenshares and inspect the changes of the state's localScreenshareStatus
  2. initially it is undefined
  3. starting a screenshare => starting
  4. when screenshare is in progress => active
  5. stop screenshare either by the stopScreenshare action or by the browser's native alert => undefined again

useRoomConnection's default options:

  1. use the sdk's useRoomConnection with a roomUrl only, like useRoomConnection(roomUrl)
  2. it should connect with the default media constraints

newJoiner and streams props in remoteParticipant

  1. console.log the state or state.remoteParticipants in your demo app
  2. join the room with 2 participants
  3. check the logs: newJoiner and streams props shouldn't be there.

Make sure your demo app works after migrating to this version 😇 .

@nandito nandito requested a review from a team October 24, 2023 14:35
@nandito nandito self-assigned this Oct 24, 2023
@nandito nandito marked this pull request as draft October 24, 2023 14:40
@nandito nandito changed the title Rename rejected status to knock_rejected Clean up room connection status Oct 24, 2023
@nandito nandito force-pushed the nandor/pan-400-rename-rejected-knock_rejected branch 3 times, most recently from bd41371 to 908a4ed Compare October 25, 2023 15:25
@nandito nandito marked this pull request as ready for review October 25, 2023 15:36
@nandito nandito force-pushed the nandor/pan-400-rename-rejected-knock_rejected branch 2 times, most recently from 9c52bbd to 3adad69 Compare October 26, 2023 08:28
This state is not used for anything, it should be safe to remove.
... and fix the missing state reset when screen share stops.
Use default localMediaConstraints' audio, video: true.
`useLocalMedia` already has a default option and the same value was
passed in the story. This commit removes this unnecessary param.
…articipants`

These props are only used internally, so no need to add them to the
useRoomConnection state.
@nandito nandito force-pushed the nandor/pan-400-rename-rejected-knock_rejected branch from 3adad69 to c2f0415 Compare October 26, 2023 10:04
@nandito nandito force-pushed the nandor/pan-400-rename-rejected-knock_rejected branch from c2f0415 to 80cd661 Compare October 26, 2023 12:44
Copy link
Contributor

@kevinwhereby kevinwhereby left a comment

Choose a reason for hiding this comment

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

awsome!

@kevinwhereby
Copy link
Contributor

kevinwhereby commented Oct 26, 2023

Currently elbow deep in typescript trying to make a constructor for these events that types them automatically 😂 😭

type RoomEventKey = keyof RoomEventsMap;
type ArgType<T> = T extends (arg: infer U) => unknown ? U : never;
type RoomEventHandler<T extends RoomEventKey> = RoomEventsMap[T];
type RoomEvent<T extends RoomEventKey> = ArgType<RoomEventHandler<T>>;
type RoomEventPayload<T extends RoomEventKey> = RoomEvent<T> extends CustomEvent<infer U> ? U : never;

function constructCustomEvent<T extends RoomEventKey>(
    eventType: T,
    eventInitDict: { detail: RoomEventPayload<T> }
): RoomEvent<T> {
    return new CustomEvent(eventType, eventInitDict);
}

SO close:
image

except this :(
image

EDIT: OK I got it, was just being too explicit with the types. I'll do up another PR for after this one, as it's purely a typing change (plus a function wrapper) it can wait until after launch

@havardholvik havardholvik merged commit ed7408e into development Oct 27, 2023
2 checks passed
@havardholvik havardholvik deleted the nandor/pan-400-rename-rejected-knock_rejected branch October 27, 2023 13:44
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.

3 participants