-
Notifications
You must be signed in to change notification settings - Fork 48
fix: Handle empty playlist #122
base: master
Are you sure you want to change the base?
Conversation
.twoColumnBrowseResultsRenderer.tabs[0].tabRenderer.content | ||
.sectionListRenderer.contents[0] | ||
.itemSectionRenderer.contents[0] | ||
.playlistVideoListRenderer.contents; | ||
.playlistVideoListRenderer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use optional chaining here but that requires node v14.5.0 or above, so I used the ternary operator to be compatible with node v8. I do recommend you bump node to v14.5.0 or above because v8 is EOL and v12 is going to EOL next year
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With optional chaining the updated code should look like this
const rawVideoList = parsed.json.contents
.twoColumnBrowseResultsRenderer.tabs[0].tabRenderer.content
.sectionListRenderer.contents[0]
.itemSectionRenderer.contents[0]
.playlistVideoListRenderer?.contents;
if (rawVideoList === undefined) throw new Error('Playlist is empty');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current changes, it would be like this with optional chaining
const playlistVideoListRenderer = parsed.json.contents
.twoColumnBrowseResultsRenderer.tabs[0].tabRenderer.content
.sectionListRenderer.contents[0]
.itemSectionRenderer.contents[0]
.playlistVideoListRenderer?.contents ?? [];
Should test for empty playlist created too? |
would be cool 😅 |
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 260 261 +1
=========================================
+ Hits 260 261 +1
Continue to review full report at Codecov.
|
i'm also more of a |
Yes, I didn't think of that before, changed in d3519ab. Do the test still need to be created? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This should be reopened when you're ready. |
not quite ready but i can at least tell the stale bot to ignore this 😉 |
This should fix #121