-
-
Notifications
You must be signed in to change notification settings - Fork 250
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
feat: add TV client endpoints and nodes to get content with default OAuth2 Login #872
base: main
Are you sure you want to change the base?
Conversation
…nd horizontallist continuation item
Regarding the failing test, locally it works. ;) |
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.
This pull request is absolutely massive, it would definitely be a lot more sensible to split it out into multiple smaller ones, especially because it contains some changes that have nothing to do with the TV nodes like the watch history stuff.
Here are a list of things that definitely need to change before it could be considered mergable (not sure it if should be considering that it adds lots of nodes and bloat that many users of the library probably won't even need) but it definitely should be split up into multiple pull requests.
src/core/clients/TV.ts
Outdated
import { HorizontalListContinuation, type IBrowseResponse } from '../../parser/index.js'; | ||
import { Parser } from '../../parser/index.js'; |
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.
Please deduplicate these imports
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.
Done here.
Maybe to make this a requirement, we could add the no duplicate imports rule to the eslint config to show errors if this happens?
https://eslint.org/docs/latest/rules/no-duplicate-imports
#session: Session; | ||
readonly #actions: Actions; |
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.
You are saving the session and the actions instances in their on variables but then in various places you still access the actions instance through this#session.actions
. Could you please pick one and use it consistently throughout the class.
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.
Replaced to use #action property.
tsconfig.json
Outdated
// "sourceMap": true, /* Create source map files for emitted JavaScript files. */ | ||
"sourceMap": true, /* Create source map files for emitted JavaScript files. */ |
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.
Please revert unrelated changes.
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.
The sourceMaps got removed some major version before, was there a specific reason for that you know?
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.
I still use them, but only locally. They were bloating the NPM package, so I decided to just disable them.
Additionally please change the commit messages to say feat instead of chore, release please looks at the commit messages so if this were to be merged, then it would get logged as a chore and not a feature implementation. |
/** | ||
* Retrieves the user's My YouTube page. | ||
*/ | ||
async getMyYoutubeFeed(): Promise<MyYoutubeFeed> { |
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.
Is this the library? If yes, maybe we should just name it getLibrary
.
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.
This seemed to be an extra browseId "FEmy_youtube", which differed from the normal Library as it contained a left tab bar with a grid on the right, based on the tab selection.
@Duell10111 This PR is quite large, so I'll be helping you with it very soon. I'm also interested in getting OAuth working again, but we may need to make improvements in a few places. |
Thank you, I would be glad to get help. |
As reaction to the block of the classic TV OAuth authentication on non TV endpoints, I tried to add Helper classes with TV Client optimization.
Related issue: #803
As I am not an expert of the Innertube API and the Youtube.js library, feel free to optimize or bring up suggestions of this PR.
Therefore this PR is created as draft. :)
Additionally regarding TV endpoints, I added a new function to adapt watched time for videos added to the watch list (history). (Issue: #825)
Further for the TV client I added a helper function to get the continuation of HorizontalList as needed for the TV Homefeed. :)