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

Type custom event payloads #103

Conversation

kevinwhereby
Copy link
Contributor

Adds a thing custom event wrapper that ensures our events align with what we define in RoomEventsMap and LocalMediaEventsMap

To test

  • I guess if it compiles and the test apps still work we're golden?

Gotchas

relies on us using RoomEvent|LocalMediaEvent, so if someone comes along and adds another handler using CustomEvent we lose the typings...

We could add a this._dispatchRoomEvent(e: RoomEvent) function to the class to solve this maybe?
But then someone could just use this.dispatchEvent(e) to get around that...

Maybe it's not a big deal

@kevinwhereby kevinwhereby changed the base branch from development to nandor/pan-400-rename-rejected-knock_rejected October 26, 2023 14:46
@kevinwhereby kevinwhereby force-pushed the kevinhanna/room-event-typing branch from d49bdf9 to 48029d5 Compare October 26, 2023 14:51
stream,
id: streamId,
isLocal: false,
hasAudioTrack: stream.getAudioTracks().length > 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unsure if this is correct... input welcome

Copy link
Collaborator

@havardholvik havardholvik Oct 27, 2023

Choose a reason for hiding this comment

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

I think this should be sufficient. It at least answers the question the prop seemingly asks hasAudioTrack. However, the track could be disabled, meaning no audio, but I honestly dont know if that is the case event for a screenshare started without audio. I would keep 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess semantically hasAudioTrack would still be true in this case even if it was disabled or muted right?

But yeah, we can find out if it becomes an issue I guess

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, we are staying true to the prop name 👍

Copy link
Collaborator

@havardholvik havardholvik left a comment

Choose a reason for hiding this comment

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

Nice! One thing I strongly recommend is to rename RoomEvent -> RoomConnectionEvent, to align what we have for LocalMedia -> LocalMediaEvent

stream,
id: streamId,
isLocal: false,
hasAudioTrack: stream.getAudioTracks().length > 0,
Copy link
Collaborator

@havardholvik havardholvik Oct 27, 2023

Choose a reason for hiding this comment

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

I think this should be sufficient. It at least answers the question the prop seemingly asks hasAudioTrack. However, the track could be disabled, meaning no audio, but I honestly dont know if that is the case event for a screenshare started without audio. I would keep 👍

@kevinwhereby kevinwhereby force-pushed the kevinhanna/room-event-typing branch from c3dd0d3 to e3503e1 Compare October 27, 2023 07:54
@kevinwhereby kevinwhereby merged commit 3df8a81 into nandor/pan-400-rename-rejected-knock_rejected Oct 27, 2023
@kevinwhereby kevinwhereby deleted the kevinhanna/room-event-typing branch October 27, 2023 07:55
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