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

restoreStateFromSnapshot after PlayerContextProvider is created #318

Closed danielr18 closed 5 years ago

danielr18 commented 5 years ago

Question: Should there be a way to call restoreStateFromSnapshot after the PlayerContextProvider is created?

At the moment, it's only used in the constructor, which totally makes sense, however if you would like to use localStorage to store the snapshots in a SSR app, you can get the snapshot after the component that renders PlayerContextProvider is mounted.

You could render PlayerContextProvider after the parent mounts, and thus you could pass the initialStateSnapshot, but I think the app would break if you have children that use playerContextFilter without having PlayerContextProvider in the tree.

What's your opinion?

benwiley4000 commented 5 years ago

Hm... maybe store the snapshot in the cookie instead of localStorage so it's accessible on the server side as well? Would that solve your use case?

And I guess I would try to get the stored snapshot synchronously before you render to avoid the other part of the issue.

It's definitely a use case we should solve, so if this doesn't seem good enough to you, I'd be happy to discuss alternatives.

benwiley4000 commented 5 years ago

Closed on accident, sorry!

danielr18 commented 5 years ago

I thought about storing in cookies, but I would also have store the playlist in the cookies (which can be bigger than the cassette snapshot). If I have playlist in localStorage, I think restoreStateFromSnapshot wouldn't set the activeTrackIndex, because at the time it's created, playlist is invalid.

benwiley4000 commented 5 years ago

Hm, I see. Do you have any ideas about a potential API? I can see why you want this.

But do you need the entire playlist for the initial render? If yes, I think maybe you should pass the whole playlist in the cookie. If you do not need the whole playlist, maybe you should only store the current track object in the cookie, and you can initialize the playlist with only one track on the server side. You can update with the full playlist later on the client side (on componentDidMount), and the PlayerContextProvider will keep you on the same track.

danielr18 commented 5 years ago

If I store the current track and cassette snapshot in cookies, I could create a playlist with n empty tracks before the activeTrackIndex and set the current track in that index for the first render, and then change the whole playlist to the one from localStorage after componentDidMount.

benwiley4000 commented 5 years ago

It's been awhile since I wrote it but if you look at this code https://github.com/benwiley4000/cassette/blob/next/packages/core/src/utils/snapshot.js#L84 I don't think you need to insert all the empty tracks in front. It should figure it out for you I think?

benwiley4000 commented 5 years ago

Arguably activeTrackIndex doesn't even need to be part of the snapshot but I guess I included it in case your playlist had multiple instances of the same track and you wanted to keep your spot

danielr18 commented 5 years ago

Not sure, unless I'm not seeing something you are aware of.

//  Example: activeTrackIndex = 7 (from snapshot)
/**
* This would try to get the source from props.playlist[activeTrackIndex]. 
* It would fail in this line: const { sources, url } = playlist[index]; I think, because playlist[index] is undefined.
*/
const currentSrc = getTrackSources(props.playlist, activeTrackIndex)[0].src;
if (activeTrackSrc === currentSrc) {
  restoredStateValues.activeTrackIndex = activeTrackIndex;
  useCurrentTime = true;
} else {
  // If we get here, it would probably work. This would try to find the index for the track, if playlist is just [activeTrackFromCookie], then newTrackIndex would be 0, and then after playlist changes to have the whole tracks, I think it would be set to 7.
  const newTrackIndex = findTrackIndexByUrl(props.playlist, activeTrackSrc);
  if (newTrackIndex !== -1) {
    restoredStateValues.activeTrackIndex = newTrackIndex;
    useCurrentTime = true;
  }
}    
danielr18 commented 5 years ago

If I remove activeTrackIndex from the snapshot, then it would go to the else condition and work.

benwiley4000 commented 5 years ago

Ah you're right! That code needs to be fixed. Maybe it can be:

const currentSrc = props.playlist[activeTrackIndex] && getTrackSources(props.playlist, activeTrackIndex)[0].src;
if (currentSrc && activeTrackSrc === currentSrc) {
danielr18 commented 5 years ago

Yes, or returning blankSources in getTrackSources if the playlist[index] is undefined.

benwiley4000 commented 5 years ago

I considered that, but I figured it might break something else, and also this is pretty much the only case where an undefined track should be possible I believe?

danielr18 commented 5 years ago

As far as I'm aware, so yes, your snippet should work.

benwiley4000 commented 5 years ago

Yep. If there are other cases they're probably bugs, so I'd rather catch them when error messages arise. :)

I can merge this fix and release a new version in a few minutes.

benwiley4000 commented 5 years ago

Does this seem like a good solution to your problem?

danielr18 commented 5 years ago

Yes, I think it's a particular case, so better to handle it in my codebase than having to change the API to support it.

benwiley4000 commented 5 years ago

The fix we discussed above is released in v2.0.0-alpha.9. Let me know if you have any further issues - and thanks as always for all your help! :smiley: