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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix author and thumbnails for autogenerated playlists #251

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

absidue
Copy link
Collaborator

@absidue absidue commented Dec 1, 2022

Pull Request Template

Description

While testing FreeTubeApp/FreeTube#2903, we noticed that autogenerated playlists like https://m.youtube.com/playlist?list=RDCLAK5uy_nxNaxtS5HwGabWeGHsAADVA4KKl5l12eQ are partially supported by this library, with it spitting out this error:
casting_error

This pull request fixes that casting issue and also fixes the author field on the playlist being empty, the only missing bit of information is the thumbnails for the playlist author (empty array), although YouTube doesn't show it either, so I think that's fine.

The changes in this pull request work well with this FreeTube pull request FreeTubeApp/FreeTube#2903 and were tested with it. Interestingly when I wanted to add a unit test to this library to test the autogenerated playlist, I ran into this error:
unit_test_error
Which is weird, because YouTube will happily return the playlist when the library is used inside of FreeTube. If I had to guess the differences are probably user agent related. Happy to add a unit test if there is a way to make it work in the unit test.

Here are the relevant lines of code in FreeTube, yes we don't have the most standard setup for YouTube.js 馃槵, see FreeTubeApp/FreeTube#2855 for some of the reasoning behind our setup. We also disable CORS entirely in Electron, so that we don't have to deal with that.

_scripts/webpack.renderer.config.js | lines 129-141

src/renderer/helpers/api/local.js | lines 18-43

src/main/index.js | lines 175-183

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@LuanRT
Copy link
Owner

LuanRT commented Dec 6, 2022

Hey, thanks for taking the time to fix this!

Interestingly when I wanted to add a unit test to this library to test the autogenerated playlist, I ran into this error.

Just tried adding a unit test with the id OLAK5uy_nGOuZnWRbeqZlch9DFLklW59Zrzyq2lx4 and it worked. So I am a bit confused, maybe there was something wrong with the id you used?

@absidue
Copy link
Collaborator Author

absidue commented Dec 6, 2022

I used this ID RDCLAK5uy_nxNaxtS5HwGabWeGHsAADVA4KKl5l12eQ (see link in the description above), which works in the mobile YouTube app and FreeTube (with this YouTube.js PR but not with ytpl because that scrapes the html) but doesn't work on the desktop YouTube website or in the unit test.

@LuanRT
Copy link
Owner

LuanRT commented Dec 7, 2022

Alright, got it. Going to merge this now, we'll see about unit tests later.

@LuanRT LuanRT merged commit f5d61d7 into LuanRT:main Dec 7, 2022
@absidue absidue deleted the autogenerated-playlist branch December 8, 2022 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants