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

Can we rename setSelectedCandidatePair to selectCandidatePair ? #178

Closed
jan-ivar opened this issue Oct 27, 2023 · 3 comments · Fixed by #183
Closed

Can we rename setSelectedCandidatePair to selectCandidatePair ? #178

jan-ivar opened this issue Oct 27, 2023 · 3 comments · Fixed by #183
Assignees

Comments

@jan-ivar
Copy link
Member

jan-ivar commented Oct 27, 2023

Sorry for not catching this in review, but...

Is it too late to rename setSelectedCandidatePair to selectCandidatePair ?

Using the right verbs often helps when defining methods.

Also, "setFoo" methods are a bit of an anti-pattern I feel (they beg the question of whether something should have been a setter attribute instead.)

cc @sam-vi

@sam-vi
Copy link
Contributor

sam-vi commented Oct 30, 2023

setSelectedCandidatePair was proposed as a natural counterpart to RTCIceTransport.getSelectedCandidatePair.

I don't think a setter attribute is a good fit here because:

  1. setSelectedCandidatePair does not immediately mutate the internal slot returned by getSelectedCandidatePair, that only happens asynchronously.
  2. setSelectedCandidatePair performs a complex operation (ICE agent actions), something that the Web Platform Design Principles recommends against attributes from performing.

Thinking also about @henbos's suggestion in #175 (review), perhaps an overall improvement, which also makes the verb more suitable, would be to change the method signature to

Promise<undefined> setSelectedCandidatePair(RTCIceCandidatePair candidatePair);

Then the sequence of operations on setSelectedCandidatePair is:

  • Perform input & state validation
  • Return a promise
  • In parallel, instruct the ICE agent to change the selected candidate pair
  • Upon completion, update the SelectedCandidatePair internal slot
  • Fire the selectedcandidatepairchange event
  • Resolve the promise

Then the application knows when the setter completes and the getter is consistent with the result. get/setParameters in RTCRtpSender have a similar structure.

The spec changes should be minimal. What do you think?

@jan-ivar
Copy link
Member Author

I don't think a setter attribute is a good fit here because:

Note I'm not proposing a setter attribute.

  1. setSelectedCandidatePair does not immediately mutate the internal slot returned by getSelectedCandidatePair, that only happens asynchronously.

Right, setSelectedCandidatePair seems a bad name for the same reason.

I'm proposing calling the method selectCandidatePair. This implies a method that is doing something (selecting) other than simply storing the value immediately.

Returning a promise seems orthogonal to naming, but I agree it would conveniently solve the issue of expected behavior by clarifying when the returned value should match

await transceiver.transport.iceTransport.selectCandidatePair(foo);
const bar = transceiver.transport.iceTransport.getSelectCandidatePair();
// foo and bar should match

@henbos
Copy link
Collaborator

henbos commented Oct 31, 2023

Renaming this method SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants