-
Notifications
You must be signed in to change notification settings - Fork 0
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
FCE-205 Refactor into smaller files #7
Conversation
…efactor-v2 # Conflicts: # e2e-tests/react-client/app/package.json # packages/ts-client/src/webrtc/webRTCEndpoint.ts # yarn.lock
…efactor-v2 # Conflicts: # e2e-tests/ts-client/app/package.json
…elongToEndpoint` to `StateManager.ts`
…StateManager.ts`
(this.stateManager.connection.signalingState !== 'stable' || | ||
this.stateManager.connection.connectionState !== 'connected' || | ||
this.stateManager.connection.iceConnectionState !== 'connected') | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please extract this big boolean to a variable and use the variable in the condition, same can apply to the 95th line, sth like
const areThereOngoingOperations = ...
const isStateInvalid = ...
if(areThereOngoingOperations || isStateInvalid) return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approach could be to create separate function and call here as:
if(this.isConnectionActvite()) { ...
(maybe with better name?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with something like this:
private isConnectionUnstable() {
const connection = this.stateManager.connection;
if (connection === undefined) return false;
const isSignalingUnstable = connection.signalingState !== 'stable';
const isConnectionNotConnected = connection.connectionState !== 'connected';
const isIceNotConnected = connection.iceConnectionState !== 'connected';
return isSignalingUnstable && isConnectionNotConnected && isIceNotConnected;
}
private isNegotiationInProgress() {
return (
this.negotiationManager.ongoingRenegotiation ||
this.stateManager.ongoingTrackReplacement
);
}
public processNextCommand() {
if (this.isNegotiationInProgress()) return;
if (this.isConnectionUnstable()) return;
this.resolvePreviousCommand();
const command = this.commandsQueue.shift();
if (!command) return;
this.commandResolutionNotifier = command.resolutionNotifier;
this.handleCommand(command);
}
if (this.commandResolutionNotifier) { | ||
this.commandResolutionNotifier.resolve(); | ||
this.commandResolutionNotifier = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
little to not important nit:
would you prefer early returns in general?
if (!this.CRN) return
this.commandResolutionNotifier.resolve();
this.commandResolutionNotifier = null;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:50_50:
I usually prefer to avoid early returns, as they make code less readable. But this is subjective opinion.
if (trackContext.simulcastConfig!.enabled) { | ||
transceiverConfig = simulcastTransceiverConfig; | ||
const trackActiveEncodings = trackContext.simulcastConfig!.activeEncodings; | ||
const disabledTrackEncodings: TrackEncoding[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this fn could be broken down to smaller functions to increase readibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I will refactor it in the future PRs
disabledTrackEncodings.push(encoding.rid! as TrackEncoding); | ||
} | ||
}); | ||
disabledTrackEncodingsMap.set(trackContext.trackId, disabledTrackEncodings); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we mutate a map passed in as an argument and return a completely other value.
please create another function to handle updating the disabled encodings
i can see its being drilled through at least 3 functions, i still think it should get refactored though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do it in future PRs
(this.stateManager.connection.signalingState !== 'stable' || | ||
this.stateManager.connection.connectionState !== 'connected' || | ||
this.stateManager.connection.iceConnectionState !== 'connected') | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approach could be to create separate function and call here as:
if(this.isConnectionActvite()) { ...
(maybe with better name?)
if ( | ||
this.negotiationManager.ongoingRenegotiation || | ||
this.stateManager.ongoingTrackReplacement | ||
) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something for separate PR. But I would vote to add a prettier rule to always use brackets. This will make expressions like this more readable.
if ( | |
this.negotiationManager.ongoingRenegotiation || | |
this.stateManager.ongoingTrackReplacement | |
) | |
return; | |
if ( | |
this.negotiationManager.ongoingRenegotiation || | |
this.stateManager.ongoingTrackReplacement | |
) { | |
return; | |
} |
if (this.commandResolutionNotifier) { | ||
this.commandResolutionNotifier.resolve(); | ||
this.commandResolutionNotifier = null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:50_50:
I usually prefer to avoid early returns, as they make code less readable. But this is subjective opinion.
* Indicates if an ongoing renegotiation is active. | ||
* During renegotiation, both parties are expected to actively exchange events: renegotiateTracks, offerData, sdpOffer, sdpAnswer. | ||
*/ | ||
public ongoingRenegotiation: boolean = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about having two functions renegotationStarted()
and renegotationCompleted()
with private variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. That's why I created this class. I will do it in future PRs
.filter((transceiver) => transceiver.sender.track?.id && transceiver.mid) | ||
.reduce( | ||
(acc, transceiver) => { | ||
const localTrackId = transceiver.sender.track!.id; | ||
const mid = transceiver!.mid!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would require a little more work, but in future refactor it would be nice to create filter functions that returns proper TS type, so no !
will be required.
I will address the remaining comments (on the old code that I moved to other files) in future PRs. For now, I don't want to interfere too much with the logic. I would like to dedicate the next PR to cleaning up the state, which is currently spread across many fields is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR;
Description
Basic refactor:
Motivation and Context
src/webrtc/webRTCEndpoint.ts
is too big and has too many responsibilities. It's hard to work with it.Types of changes
Refactor (non-breaking change)