Playlist Order

[vinnycodes]

When a playlist is rendered, it is not always in order. Looking for a solution to try and create an "orderedPlaylist" option.

[kiahreading]

I have run into that same issue. Playlist order is not stable, and when the order is wrong, a video (for example the 3rd video) is shown in the 1st position, highlighted as if it was playing.

It is a really nice library, just hoping for this fix so that I can use it.

[vinnycodes]

I have a pull request for this and should be pushing a solution by the end of the weekend.
Just need to go over it and QA it.

[stephenscaff]

I'll take a look.

[stephenscaff]

Oh, okay, I think the issue is that the Promises for each requested vimeo id are not properly chained, so they are resolving when they do, which isn't necessarily the order of the requests.

The issue happens right at line 219, vidInfo.then((obj) =>.

I'm looping over each id, making a fetch to vimeo, then creating a doc frag to render each item to a template before adding it back to the DOM. Need to ensure the promises are chained in the correct order.

Will work on a solution shortly. But, if you have a solution @vinnycodes, feel free to send that PR (thanks!).

[vinnycodes]

Hey @stephenscaff!
Great package overall so thank you for it.

I found the issue at line 129 as well in how the fetch is happening asynchronously. I'm wrapping up some client work today and should have it back to you. I went ahead and used Promise.all to and that helped.

I'll send it back assap.

[stephenscaff]

@vinnycodes Yep. Promises.all is the move imo. Need to beef up error handling a bit too, if an id doesn't exist. Thanks for your help. Will also try to branch a solution if I get a sec today.

Thanks again.

[stephenscaff]

@vinnycodes I did a fix, here's the PR:.
The branch is fix-vids-order if you wanted to take a look.

Wraps fetches in a Promise.all. Created a util to abstract. Also added some new options for playlist-items that you can see in readme. Though should probs rethink that a bit.

Anyway, will merge the PR later unless you think you have something better? Im sure my solution could be better but I just had a baby girl and my brain is a tad fried.

[vinnycodes]

@stephenscaff
First of all, congrats to you and yours! Hope it's a wonderful eventful time despite keeping you fried. :)

Thank you for taking time away to look at this.

Your PR looks very clean. I like how you did it. Here's the PR I have

I solely added it for the reasons below. Of course feel free to use whatever you would like and throw away what you don't.

• I added a shuffle option that keeps your original "buildPlaylist" function when true and added a "buildOrderedPlaylist" that runs through a Promise all instead. After seeing your code, this can probably be tweaked to be more succinct.

• I added a description option. When set to true, there will be a description of the current video under the video player.

Thank you again and hope this can better the project. It's been really helpful for me.

[stephenscaff]

@vinnycodes Thanks bud! appreciate the kind words.

Few thoughts / questions:

1. First, question about Promise.all approaches

Is it better to loop over an array of promises, like in your solution, or is it better to have a single Promise and loop over the PromiseResult array, as in my solution?

In your solution, we loop over the fetch requests to create an array of Promises.

[Promise, Promise, Promise, Promise, ....]

Then, we use Promises.all to loop over that array of Promises.

In my solution, we loop over the requests inside a Promise.all (Promise.all(playlist.map(request =>...))) to create a single Promise housing the requests inside a PromiseResult Array:

Promise {
 [[PromiseState]]: '"fullfilled"
 [[PromiseResult]]: Array() // our requersts
}

that we can then resolve and build the frag for output.

Which is better? Which is more performant?
My quick research into the matter isn't producing anything definitive.

Let me know what you think.

2. Second, we should swallow up 404 request errors

Adding an undefined check so a 404 error (from bad id) won't kill the app.
Added that check fix-vids-order branch here. Does the trick.

3. Finally, on Shuffle

Great idea. Thinking it should maybe be a control / toggle though. In the same way we have playlist controls for next, prev, fullscreen, perhaps Shuffle should just be a button/trigger.

Then, the shuffle trigger could just randomly reorder the existing playlist (rather than shuffle the list before it's built.

Does that make sense?

[vinnycodes]

Hey @stephenscaff ! You're welcome.

1. I do not think it would make a difference except for how it is read by the person using the code. The reasoning behind my approach was that I wanted to keep the new function I was creating as close to the original one. Also, and I could be wrong about this, it is essentially doing the same thing except it doesn't utilize ".then". But again, this was mostly done for readability between the "buildPlaylist" and the new "buildOrdered playlist. I don't think there'd be a performative issue unless we start looking at a large amount of videos.

2. Thank you! I completely forgot to add a guard clause.

3. That's a great idea and didn't even consider that. This sounds like it would clean up a good amount of the code I was trying to add and would keep this succinct. Would you find it better to add a function that watches the "shuffle" variable and randomizes on true or just randomize it someone in the code.

Thank you again. I'm learning a lot on this one.

[DerekBuntin]

@stephenscaff & @vinnycodes can this be merged?

I'm currently experiencing this same issue and would like to get it resolved.