Player.playSegment doesn't return a promise

[adi518]

I encountered a play/pause exception in my app, since it doesn't handle these methods correctly. player.play() is an async method, which returns a promise, as expected. However, it doesn't do so with player.playSegment(), which means you cannot obtain the play promise to later make sure you can pause the player only once it's playing.

https://github.com/bbc/peaks.js/blob/75f9098661c259d54b0da8ef2b7793e77d4e88a4/src/player.js#L188

[adi518]

After further inspection, there's a bigger issue. This function should return a promise, but not that of the play method, but a promise that represents the end of a segment playback. Atm, I have a use-case where I need to pick several segments and have the player play just those segments. However, simply looping these segments and calling playSegment on each of them will not work or have unexpected behavior. I think the design of the API should incorporate promises, which can absolve consumers from having to tangle events together. Also, I noticed peaks.once API is not covered by the docs. This API can be crucial to create custom behavior.

[chrisn]

playSegment() should return the same as play() - the promise would indicate whether playback started successfully. If playSegment() returned a promise and you choose to play in looped mode, the promise would never resolve. And we'd have to make it resolve if playback stops for any reason (pause, network error, etc). The player.ended event allows you to respond when playback reaches the end of the segment. But I can see that the code you'd have to write to do that is a little complex.

I agree that we should add peaks.once to the documented API, possibly with any other relevant EventEmitter functions.

[adi518]

The problem we are facing with events is concurrency. Lots of things can happen until an event fires, which can then interfere with the logic we are after. Events are evil necessity, but at the same time, we can maybe try and abstract them into promises. For instance, if some logic fails, we can reject the promise, similar to Chrome's native exception when calling pause before play is resolved. It could make things easier to reason about.

As for looping a segment. I think you can resolve the promise with a one-off pause listener.

[chrisn]

My feeling is that this is solvable using event listeners and some stateful logic in your application. I'd like to see an example before considering changing the Peaks API.

[adi518]

I'm tinkering with my fork to a return a promise from playSegment. I ran into this condition, which I don't understand: https://github.com/bbc/peaks.js/blob/75f9098661c259d54b0da8ef2b7793e77d4e88a4/src/player.js#L192. I'm guessing it has something to do with an edge case of sort. If it's not playing, why pause it?

[chrisn]

You're right, there's no need to pause there.

[adi518]

Awesome, that's one simpler implementation. :)

[chrisn]

I'll close this, but please reopen if there's still a problem.