Skip to content

Conversation

BillCarsonFr
Copy link
Member

Proposal to revert that change b0eb566 and keep only static variant of state instead of having one of the observable variant beeing him self another observable.

 | { state: "FailedToStart"; error: Error; transport: LivekitTransport }
  | {
      state: "ConnectedToLkRoom";
      connectionState$: Observable<ConnectionState>;
      transport: LivekitTransport;
    }

I think it makes it easier to use. I added back the test that was deleted
2c66e11
image.png

If this proposal is not accepted, then at least the deleted test should be added back using the new API

@BillCarsonFr BillCarsonFr requested a review from a team as a code owner October 15, 2025 08:19
@BillCarsonFr BillCarsonFr requested review from robintown and removed request for a team October 15, 2025 08:19
@BillCarsonFr BillCarsonFr changed the base branch from toger5/voip-team/rebased-multiSFU-bump-to-new-js-sdk-version to voip-team/rebased-multiSFU October 15, 2025 08:19
[],
);

scope
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we could have a switch map from a new startLifecycle observable if you prefer

@BillCarsonFr BillCarsonFr changed the title Revert transport state to avoid having an embedded observable in on of the variant Revert transport state to avoid having an embedded observable in one of the variant Oct 15, 2025
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

I don't agree that this is necessarily easier to use. As the diff shows it's basically a matter of map versus switchMap, and meanwhile what was a clean declarative specification with almost nothing to test becomes imperative + with poorer locality of behavior.

There are also some things that become easier to do when state is not flattened in this way, such as maping with ideal performance when you don't care about the ConnectionState, or accumulating a state that resets whenever you're outside the ConnectedToLkRoom phase.

This is not high-temperature feedback and you're welcome to merge this if you still like it better, but I just wanted to give context as to why, more generally, I think we shouldn't shy away from dynamic values that contain other dynamic values when they're also a valid way of modelling a problem.

Comment on lines +254 to +255
.behavior<ConnectionState>(connectionStateObserver(livekitRoom))
.subscribe((livekitState) => {
Copy link
Member

@robintown robintown Oct 17, 2025

Choose a reason for hiding this comment

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

This subscribe call would need a .pipe(this.scope.bind()) to avoid risk of leaking, for the record

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