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 bidirectional communication to exchange texts #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nidhal-labidi
Copy link
Contributor

@nidhal-labidi nidhal-labidi commented Jan 13, 2025

Checklist

Reviewer

  • I've checked out the code and tested it myself.

Changes

Check the issue #107

Signed-off-by: nidhal-labidi <nidhal.labidi@compose.us>
@nidhal-labidi nidhal-labidi requested a review from Narigo January 13, 2025 09:48
Copy link
Member

@Narigo Narigo left a comment

Choose a reason for hiding this comment

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

Not sure whether it's better to expose APIs for using bi-directional AND allowing to send just in one direction instead of only bi-directional then...

};

private configureDataChannel = () => {
if (this.dataChannel == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.dataChannel == null) {
if (this.dataChannel === null) {


private configureDataChannel = () => {
if (this.dataChannel == null) {
this.changeState('error', 'dataChannel is null. Unable to setup the configure it!');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.changeState('error', 'dataChannel is null. Unable to setup the configure it!');
this.changeState('error', 'dataChannel is null. Unable to configure it!');

Comment on lines +121 to +127
if (this.dataChannel == null) {
this.changeState(
'error',
'dataChannel is null. Unable to setup the listeners for the data channel'
);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Either === null or something like this:

Suggested change
if (this.dataChannel == null) {
this.changeState(
'error',
'dataChannel is null. Unable to setup the listeners for the data channel'
);
return;
}
if (!this.dataChannel) {
this.changeState(
'error',
'dataChannel is not defined. Unable to setup the listeners for the data channel'
);
return;
}

@@ -70,6 +71,11 @@ export class FlottformChannelHost extends EventEmitter<FlottformEventMap> {
this.openPeerConnection = new RTCPeerConnection(this.rtcConfiguration);

this.dataChannel = this.createDataChannel();
if (this.dataChannel) {
//this.dataChannel.bufferedAmountLowThreshold = this.BUFFER_THRESHOLD;
Copy link
Member

Choose a reason for hiding this comment

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

dead code

Suggested change
//this.dataChannel.bufferedAmountLowThreshold = this.BUFFER_THRESHOLD;

@@ -5,7 +5,7 @@
import { sdpExchangeServerBase } from '../../../api';

let currentState = $state<
'init' | 'connected' | 'sending' | 'done' | 'error-user-denied' | 'error'
'init' | 'connected' | 'text-transfered' | 'sending' | 'error-user-denied' | 'error'
Copy link
Member

Choose a reason for hiding this comment

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

typo

Suggested change
'init' | 'connected' | 'text-transfered' | 'sending' | 'error-user-denied' | 'error'
'init' | 'connected' | 'text-transferred' | 'sending' | 'error-user-denied' | 'error'

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the host page have some change as well (looking for text-received event?)

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.

Allow Bidirectional Text Transfer using Flottform
2 participants