benwiley4000 / cassette

📼 A flexible media player component library for React that requires no up-front config
https://benwiley4000.github.io/cassette/styleguide
MIT License
185 stars 28 forks source link

Snapshot not updating PlayerContextProvider state when playlist changes. #437

Open robo220 opened 4 years ago

robo220 commented 4 years ago

Hi there

In my application the initial value of the playlist prop is an empty array. After a user interaction the playlist is finally populated with tracks. The snapshot remains unchanged but could still potentially have state like the currentTime for example. Similar to one of the cassette demos, the snapshot is initialized via my component's constructor. However unfortunately the state of PlayerContextProvider is not updated to reflect what's in the snapshot.

The snapshot feature works perfectly if I pass down the populated playlist for the first time rather than having an initial empty array.

  <PlayerContextProvider
                playlist={audioPlayer.get('playlist').toJS()}
                initialStateSnapshot={this.snapshot}
                onStateSnapshot={shot => {
                    localStorage.setItem('media_snapshot', JSON.stringify(shot));
                }}>
                <MediaPlayerControls
                    controls={[
                        Play
                    ]}
                />
                <div>
                    <h3>Select a track:</h3>
                    <PlaylistMenu/>
                </div>
            </PlayerContextProvider>

I don't see any of the potential warnings in the console which could potentially be logged here https://github.com/benwiley4000/cassette/blob/next/packages/core/src/PlayerContextProvider.js#L142.

I've read https://benwiley4000.github.io/cassette/styleguide/#dont-mutate-playlists and am aware it's important for the playlist prop to be immutable. My playlist variable is managed via redux.

Does anyone know where I could be going wrong here? Or could this be a potential new feature?

EDIT: I think it's because the initial state is restored in the constructor and no where else https://github.com/benwiley4000/cassette/blob/next/packages/core/src/PlayerContextProvider.js#L149. Which is guess makes sense seeing as the prop is called initialStateSnapshot. Have I got this correct? Or is there still a way to update the state after a change in the playlist

Cheers

benwiley4000 commented 4 years ago

Thanks for the question! The answer is that it was intended behavior for the initialStateSnapshot to be read only when the component first mounts. The issue was brought up in #318 before and we ended up solving for a more specific use case.

I'm wondering about your use case. Is it possible for you to prevent rendering the component before you have the snapshot? If not, is it possible for you to force the component to re-mount by passing a key prop which changes when the initialState is available?

Let me know if either of those solutions works for you, and if not, we can find one that does.

robo220 commented 4 years ago

@benwiley4000 thank you for your fast reply.

It turns how, as you suggested, passing a key prop to force the component to remount has done the trick.

robo220 commented 4 years ago

@benwiley4000 Orignally passing down a different key to the component was a great idea as it allowed the snapshot feature to work. The problem we have now it that we'd like to move the PlayerContextProvider further up our component tree and don't want the child components from re-rendering again. Unfortunately preventing the component from rendering before the playlist has been retrieved isn't an option for us.

benwiley4000 commented 4 years ago

@robo220 Hm, good problem. I have an idea about a solution, but could you give me a little more info about what's going on in your app just so I understand? What kind of app is it? What sort of input is the user providing in order for the playlist to be located? Is the snapshot also unknowable before the component has loaded, or is it just the playlist that you don't have yet? I wonder if the solution would be to allow passing the playlist as a Promise, and just initialize the component context state once the playlist has loaded, but still require the snapshot when the component first mounts. If it's really a matter of the snapshot being unavailable on the first render, I would be curious to know why.

benwiley4000 commented 4 years ago

An alternative would be to reset the context state every time the identity of the initialStateSnapshot gets updated, but that seems like it would almost never be the desired behavior, and I'm trying to imagine a scenario where it would be what the user wants.

robo220 commented 4 years ago

@benwiley4000 The user is presented with the player but the player does not have information regarding the playlist tracks or snapshot until they click play. Once the user has clicked play ajax calls are made to retrieve the playlist and snapshot. The user can browse the site and select another playlist which would have a different snapshot etc. We do this for efficiency reasons and also to do check on the user before actually presenting them with the tracks within the playlist.

With regards to your last comment I think that might solve the problem. Or perhaps having something like updateStateSnapshot? or maybe `snapshot' in the same way playlist can be updated it'll be cool if snapshot could be updated.

What we're currently doing is storing just the trackIndex and currentTime server side and manually calling the seekComplete and selectTrackAction when the new playlist has loaded and has been passed to PlayerContextProvider.

robo220 commented 4 years ago

@benwiley4000 just had a thought... maybe I can't use the snapshot feature regardless.

We only store the trackIndex and currentTime server side. I can't seem to find a way to create a 'blank' snapshot. My intention was to create a snapshot object and replace the trackIndex and currentTime with the ones from the db. We can't really store the whole object server side as it'll be too large. It looks like using the snapshot in this way is discouraged. Have I got this right?