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

feat: useIsPlaying hook and isPlaying() method #2040

Merged
merged 4 commits into from
Jun 28, 2023

Conversation

fivecar
Copy link
Contributor

@fivecar fivecar commented Jun 24, 2023

This addresses some of the trickiness and confusion that users may have in v4's API, where State.Buffering spans both playing and paused states (whereas in v3, you could just const playing = state === State.Playing || state === State.Buffering). #2036 shows the subtlety of doing this right.

This PR adds two things to the public API:

  • useIsPlaying: a hook that will likely be used by anyone managing a Play button. It correctly tells you when the button should be shown as "playing" and when it should be shown as "buffering."
  • isPlaying(): an async function that gives you the most current playing state. This is needed for cases where you can't use a hook (e.g. outside of React components) or for cases where a hook's delay (due to subscriptions to events) isn't enough for you to make the right fork in your code.

I've added Javadoc-style comments, but am unsure where to update The Real Documentation™. I've also updated the example app for this simpler way of doing things.

@puckey
Copy link
Collaborator

puckey commented Jun 26, 2023

This is a really good addition to the library - thanks!

Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

Great addition! This will be helpful to those building apps -- and allows us to update this in the future if there's a need.

Looks like the CI is failing on a TS error, are you able to resolve this @fivecar ?

@fivecar
Copy link
Contributor Author

fivecar commented Jun 26, 2023

Done, @dcvz. Apparently had an empty blank line at the end of index.ts.

src/hooks/useIsPlaying.ts Outdated Show resolved Hide resolved
src/hooks/useIsPlaying.ts Outdated Show resolved Hide resolved
@fivecar fivecar requested a review from jspizziri as a code owner June 26, 2023 18:22
@fivecar
Copy link
Contributor Author

fivecar commented Jun 26, 2023

@puckey : made both updates. PR now returns undefined for both useIsPlaying() as well as isPlaying() when their states are undefined.

Also added docs.

@puckey
Copy link
Collaborator

puckey commented Jun 27, 2023

useIsPlaying() is not returning undefined for playing & bufferingDuringPlay yet – it would need to be something like:

function determineIsPlaying(playWhenReady?: boolean, state?: State) {
  if (playWhenReady === undefined || state === undefined) {
    return { playing: undefined, bufferingDuringPlay: undefined };
  }

  const isLoading = state === State.Loading || state === State.Buffering;
  const isErrored = state === State.Error;
  const isEnded = state === State.Ended;

  return {
    playing: playWhenReady && !(isErrored || isEnded),
    bufferingDuringPlay: playWhenReady && isLoading,
  };
}

Also, as it is I think the typescript return type of isPlaying() will unnecessarily be Promise<undefined | boolean>. So it might be an idea to do the following to fix that:

export function useIsPlaying() {
  const { state } = usePlaybackState();
  const playWhenReady = usePlayWhenReady();
  return playWhenReady === undefined || state === undefined
    ? { playing: undefined, bufferingDuringPlay: undefined }
    : determineIsPlaying(playWhenReady, state);
}

function determineIsPlaying(playWhenReady: boolean, state: State) {
  const isLoading = state === State.Loading || state === State.Buffering;
  const isErrored = state === State.Error;
  const isEnded = state === State.Ended;

  return {
    playing: playWhenReady && !(isErrored || isEnded),
    bufferingDuringPlay: playWhenReady && isLoading,
  };
}

export async function isPlaying() {
  const [playbackState, playWhenReady] = await Promise.all([
    TrackPlayer.getPlaybackState(),
    TrackPlayer.getPlayWhenReady(),
  ]);
  const { playing } = determineIsPlaying(playWhenReady, playbackState.state);

  return playing;
}

Finally, why not include the full object returned from determineIsPlaying() in isPlaying()?

We now return undefined in all cases where the state is uncertain, and we also return both `playing` and `bufferingDuringPlay` from `isPlaying()`.
@fivecar
Copy link
Contributor Author

fivecar commented Jun 27, 2023

@puckey : Ok, I've made all changes.

  1. useIsPlaying now returns double undefineds if either PlaybackState or PlayWhenReady aren't known.
  2. isPlaying now returns both playing and bufferingDuringPlay, instead of just one of them.

Note that the original Promise<boolean|undefined> from isPlaying was in response to code review feedback that I should do that, IIRC. Basically return undefined whenever the state isn't fully known. I think I've now changed things to where everyone's feedback is accounted for. LMK.

@tsheaff
Copy link

tsheaff commented May 10, 2024

I know it's been almost a year since this came out, but just want to thank @fivecar for this excellent PR. Extremely helpful!

@fivecar fivecar deleted the use_is_playing branch August 29, 2024 06:49
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.

4 participants