Change room type

[timmydoza]

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

• I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

• N/A

Description

This PR changes the type definition of the room property that is returned by the useVideoContext hook: https://github.com/twilio/twilio-video-app-react/compare/change-room-type?expand=1#diff-5f7074762bdd254d26eb256ea66a88dbc94139ca26a510d9c3d3015817b4ea78R28

The type is being changed from Room to Room | null.

Previously, when the room type was just Room, we would use a default value of new EventEmitter() as Room for the room property before the user connected to a room, and after they disconnect from a room (see this line).

I don't think that it is correct to use an empty event emitter as the value for room when the user is not connected to a room. Casting the empty event emitter to the Room type can make developers think that properties of the room object (like sid or localParticipant) are present when they actually aren't. In short, defining the room property as type Room doesn't accurately describe what the room property is.

Using a type of Room | null can slightly increase the complexity of some hooks and components (some hooks and components now have to check for the existence of a room), but this type definition better describes what values are available when a user is not connected to a room. With this new type definition, room properties like sid or localParticipant may not exist, which is accurate.

Burndown

Before review

• Updated CHANGELOG.md if necessary
• Added unit tests if necessary
• Updated affected documentation
• Verified locally with npm test
• Manually sanity tested running locally
• Included screenshot as PR comment (if needed)
• Ready for review

Before merge

• Got one or more +1s
• Re-tested if necessary

[charliesantos]

LGTM. But curious, why are we changing this now? Was it causing some issues, or just doing some cleanup?

[timmydoza]

LGTM. But curious, why are we changing this now? Was it causing some issues, or just doing some cleanup?

Good question! Prior to the Conversations work, there weren't any components or hooks that needed to know when the real room object was available. But in the new ChatProvider, we need to know when the room sid is available: https://github.com/twilio/twilio-video-app-react/pull/438/files#diff-f02d1dbe4da44e0bcda8aba04e3ee7a1bc60ab5aadfcb40b7fab3a638d495f32R66

So changing the room type in this PR does make the Conversations work easier. When new EventEmitter() as room was the default value, it looked like room.sid would always be present, even when it wasn't there.