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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Playlist): Only try extracting the subtitle for the first page #465

Merged
merged 1 commit into from
Aug 6, 2023

Conversation

absidue
Copy link
Collaborator

@absidue absidue commented Aug 6, 2023

Playlist continuations don't have the header so we should only try extracting the subtitle for the first playlist page.

closes #464
bug introduced in #458, which added support for the subtitle

@@ -36,7 +36,7 @@ class Playlist extends Feed<IBrowseResponse> {
this.info = {
...this.page.metadata?.item().as(PlaylistMetadata),
...{
subtitle: header.subtitle,
subtitle: header ? header.subtitle : null,
Copy link
Contributor

@ChunkyProgrammer ChunkyProgrammer Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this file uses header?. instead so i'm not sure if header?.subtitle would be preferred

Ex:
can_share: header?.can_share,
can_delete: header?.can_delete,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using header?.subtitle would result in it returning undefined, which would change the type of the subtitle property from Text | null to Text | null | undefined. This could be mitigated by doing header?.subtitle ?? null, but due to YouTube.js targetting ES2016, it gets transpiled to the rather verbose: (_b = header === null || header === void 0 ? void 0 : header.subtitle) !== null && _b !== void 0 ? _b : null,.

I can change the type of subtitle to be Text | undefined, if you would prefer that over null, I definitely don't want both null and undefined being possible?

@LuanRT LuanRT merged commit e370116 into LuanRT:main Aug 6, 2023
@absidue absidue deleted the fix-playlist-subtitle branch August 7, 2023 06:09
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.

5.8.0 header.subtitle failed due to header being undefined in Playlist constructor
3 participants