gilbarbara / react-spotify-web-playback

A simple player for Spotify's web playback
https://react-spotify-web-playback.gilbarbara.dev/
MIT License
321 stars 69 forks source link

Player instability when refreshing token? #138

Closed gianpietro1 closed 1 year ago

gianpietro1 commented 1 year ago

Describe the bug

I have the player defined like this:

      <SpotifyPlayer
        callback={handleCallback}
        persistDeviceSelection
        token={validSpotifyToken}
        uris={['spotify:playlist:...']}
      />

The validSpotifyToken is refreshed every 5 tracks using a refresh call in the callback function (whenever 5 track_update events pass)

When the player runs with the original token, songs are skipped normally:

initialGIF

However, as soon as the token is refreshed, it seems the device changes (the devices icon is also different) and there is some instability when skipping tracks (the previous track appears again for an instant one or two times):

afterRefreshGIF

I've even tried wrapping the player in a div that depends on the token as a key (as in the example), but I'm getting a whole reload of it when the token is updated.

Am I'm doing something wrong? or is this a bug?

Your minimal, reproducible example

https://codesandbox.io/s/interesting-greider-xtk1gw?file=/src/App.tsx

Steps to reproduce

I've tried to make a simple reproduction using a fork of the CodeSandBox example:

  1. Make sure you delete any store tokens from localStorage
  2. Enter a valid token.
  3. Pick an album or playlist and skip tracks, they will skip normally, you will get a "new track!" log per track.
  4. Enter a second valid token in the provided box that says "2nd token".
  5. Skip tracks, some of them will produce many "new track!" logs per track.

Expected behavior

I'm not sure how this should work, from my point of view it would be ideal that the player is kept without a new re-render.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

react-spotify-web-playback version

v0.12.1

TypeScript version

No response

Build tool

create-react-app

Additional context

No response

gilbarbara commented 1 year ago

Hey @gianpietro1

I couldn't reproduce it with the codesandbox you provided.

To be sure, open the preview in a new tab instead of testing in the side panel. The side panel preview runs in an iframe, and that can confuse the web playback SDK.

gilbarbara commented 1 year ago

By the way, the player shouldn't change the device when updating the token. Unless you re-mount the component multiple times and the SDK fails, it will not change automatically...

gianpietro1 commented 1 year ago

@gilbarbara thanks for taking a look. I've been checking my code extensively, ensuring no other state updates bothered the player, and again narrowed the problem down to using a dynamic token controlled by state within the player. If I use a static token, problem disappears completely.

I followed your recommendation of using a new tab for the codesandbox, and indeed the problem does not appear at first, but if you keep playing with new tokens for a little while, it will eventually happen.

In the following video, you will see that the first refresh token changes the device icon, it seems it added a new web player? (you can see this effect at 0:34 approx.) The tracks oscillation does not happen immediately, but after entering a couple of updated tokens, it starts happening (please watch the whole video, problem starts around 1:30)

https://user-images.githubusercontent.com/10046142/220831812-f185d3d8-926a-435e-a519-862aec12568f.mov

Thanks!

gilbarbara commented 1 year ago

Hey @gianpietro1 Yeah, the library is re-initializing the player when the token changes.

I've already started working on a solution, but I've to refactor the tests, so that it might take some time.

gianpietro1 commented 1 year ago

Hi @gilbarbara, that's good news! thanks for taking the time and please let me know if I can help with testing any early fix.

gilbarbara commented 1 year ago

Hey @gianpietro1

Can you add this dependency in your sandbox and check if it works? https://pkg.csb.dev/gilbarbara/react-spotify-web-playback/commit/b9259303/react-spotify-web-playback It's a custom package that uses the code from the PR

Another approach would be to add a getOAuthToken prop that accepts an async function that the player will call when it needs a token, so there's no need to generate tokens by hand. But that implementation is way more complicated...

gianpietro1 commented 1 year ago

@gilbarbara I updated the package with your fix and now it works as expected, token is refreshed without reinitializing the player. Thanks so much!! 💯

gilbarbara commented 1 year ago

Hey @gianpietro1 I've added the secondary solution as well in v0.13.0. I recommend you use that instead of refreshing tokens manually every five songs.

gianpietro1 commented 1 year ago

Thanks @gilbarbara , so I would only need to pass the function that returns a new token? probably I'm doing something wrong but it is not working for me in 0.13.0.

My player is set like this:

              <SpotifyPlayer
                callback={handleCallback}
                persistDeviceSelection
                syncExternalDevice
                offset={playlistOffset}
                token={validSpotifyToken} // first token from state, probably don't need to refresh this one anymore?
                uris={[activePlaylist.uri]}
                getOAuthToken={periodicRefreshToken}
              />

Then the periodicRefreshToken function is something like this, it gets the refresh token value from local storage and uses it for calling the Spotify API through another "refreshToken" function:

  const periodicRefreshToken = async () => {
    const savedRefreshSpotifyToken = localStorage.getItem(
      'spotifyRefreshToken',
    );
    if (savedRefreshSpotifyToken) {
      const newToken = await refreshToken(savedRefreshSpotifyToken);
      return newToken;
    }
  };

Token is returned, but the player keeps loading. Maybe I'm over complicating things?

gilbarbara commented 1 year ago

Hey @gianpietro1

I've added an example to the README on how to use getOAuthToken. It uses the same pattern as the Spotify SDK, providing a callback parameter that you call with the token instead of returning it.

And you need to update the token too, since it is used to communicate with the Spotify API.

gilbarbara commented 1 year ago

You could use your manual refresh as before, no problem at all. Up to you.

gianpietro1 commented 1 year ago

It's clear for me now, thanks for the example!