w3c / webrtc-extensions

A repository for experimental additions to the WebRTC API
https://w3c.github.io/webrtc-extensions/
Other
58 stars 19 forks source link

Can we rename setSelectedCandidatePair to selectCandidatePair ? #178

Closed jan-ivar closed 10 months ago

jan-ivar commented 11 months ago

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 commented 11 months ago

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 https://github.com/w3c/webrtc-extensions/pull/175#pullrequestreview-1701469462, 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:

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 commented 11 months ago

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 commented 11 months ago

Renaming this method SGTM