-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
refactor!: overhaul core classes and remove redundant code #388
Conversation
This is leftover code from previous versions. It had many problems and it is no longer required.
As those classes contain purely static properties and have no state, they could be converted to const variables and functions instead. While they wouldn't get treeshaken, as they are referenced by other parts of YouTube.js that don't get treeshaken, it would still benefit projects that import YouTube.js and use a bundler. Bundlers can inline functions and variables and they also tend not to mangle the names of class properties (javascript is very dynamic so renaming class properties could break stuff), but they'll happily mangle the names of functions and variables. Just thought I would mention it, as this is a new feature, so making changes to this PR now wouldn't be breaking any existing functionality. you could use a naming scheme like this (feel free to pick a completely different one of course): alternatively you could also just use build and PATH and then namespace the imports, would still be better than (unnecessarily) wrapping them in a class. Here is my manual "bundling" of the BrowseEndpoint based on the YouTube.js output and what I've seen bundlers do: class with static props: class B{static build(o){return{...{browseId:o.browse_id,params:o.params,continuation:o.continuation,client:o.client}}}}B.PATH='/browse' function and variable (these will probably also get inlined if they only get used once, so the Guide one for example): const P='/browse';function b(o){return{...{browseId:o.browse_id,params:o.params,continuation:o.continuation,client:o.client}}} |
@absidue I won't be going with pure functions such as Here's an example: // ...
export const BrowseEndpoint = {
PATH: "/browse" as const,
/**
* Builds a `/browse` request payload.
* @param opts - The options to use.
* @returns The payload.
*/
build: (opts: BrowseEndpointOptions): IBrowseRequest => {
return {
...{
browseId: opts.browse_id,
//....
},
};
}
};
Let me know if you have anything else in mind. |
This should improve the situation for bundlers.
Looks good to me 😄 |
This should reduce confusion for those looking at the codebase for the first time.
Should probably rename this to something else. I ended up refactoring way more than I first intended. |
YouTube will sometimes not return the filter chips, which is normal. But then that would cause the test to fail :/
What
To request video info, YouTube.js used to rely on a function from
Actions
calledgetVideoInfo
. In fact, it was the only function that could make requests to/player
in v1 - v2.After a while,
Actions#execute
was implemented, thus renderinggetVideoInfo
redundant. In this PR, that function is removed and endpoints are organized into individual files so we don't have to declare a payload object every time we want to send a request.Additional Improvements
Player.ts
- Client version is now appended to deciphered URLs.HTTPClient.ts
- SetuserAgent
,osName
,osVersion
andplatform
for Android clients.userAgent
is only included in the payload of Android clients.Clients
namespace.Mixins
namespace.