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

feat!: feature complete contents parser #44

Merged
merged 62 commits into from
Jun 15, 2022
Merged

feat!: feature complete contents parser #44

merged 62 commits into from
Jun 15, 2022

Conversation

Wykerd
Copy link
Collaborator

@Wykerd Wykerd commented Apr 29, 2022

Feature complete contents parser

Description

Add parsers for all known renderers and provide complete typescript definitions for all of them.

This allows the library to parse all of the contents instead discarding a lot of the valuable information.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

I will attempt maintain backward compatibility with the existing APIs

TODO:

  • Common renderers for all endpoints
  • Integrate parsers into implementation whilst maintaining backward compatibility
  • Typescript declarations
  • Tests

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

Wykerd added 2 commits April 29, 2022 21:37
This is useful for getting results other than videos, like playlists and
channels.
@LuanRT
Copy link
Owner

LuanRT commented Apr 29, 2022

Awesome! This looks very promising, thanks for contributing.

@Wykerd
Copy link
Collaborator Author

Wykerd commented Apr 29, 2022

Just to confirm: This library uses snake_case for fields and camelCase for methods?
I've been using camelCase exclusively, so I'll need to refactor if this is the case.

@@ -283,7 +283,7 @@ async function search(session, client, args = {}) {
switch (client) {
case 'YOUTUBE':
if (args.query) {
data.params = Proto.encodeSearchFilter(args.options.period, args.options.duration, args.options.order);
data.params = args.options.hasOwnProperty('params') ? args.options.params : Proto.encodeSearchFilter(args.options.period, args.options.duration, args.options.order);
Copy link
Owner

Choose a reason for hiding this comment

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

I think this won't be necessary, I've been working on the protobuf definitions for more search filters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I could tell the channels and playlists are only returned if you do not send any params, but maybe I'm missing something?

Copy link
Owner

Choose a reason for hiding this comment

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

Yup, that's correct.
But I assume that happens because the “type” search filter is missing in the protobuf definition, so YouTube is basically defaulting to “Video” when it should be “All”.

Copy link
Owner

Choose a reason for hiding this comment

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

@Wykerd

This should be fixed in commit 43f3c3f

@LuanRT
Copy link
Owner

LuanRT commented Apr 29, 2022

Just to confirm: This library uses snake_case for fields and camelCase for methods? I've been using camelCase exclusively, so I'll need to refactor if this is the case.

Yup, that's right.

Wykerd added 11 commits April 29, 2022 23:22
Added common renderers used when searching artists
These are needed for channel tabs: Videos, Playlists, Community,
Channels

Additionally, do not merely return text as string, since they may
include links which may be navigated to
Channels should be viewable when not logged in, also added 'navigation'
type for use in NagivationEndpoint in the future.
The HomeFeed class remains compatible with the existing API
Copy link
Collaborator Author

@Wykerd Wykerd left a comment

Choose a reason for hiding this comment

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

This adds typescript as a development dependency

Copy link
Collaborator Author

@Wykerd Wykerd left a comment

Choose a reason for hiding this comment

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

I believe this is related to issue #21 which also suggested it not be written by hand!

@LuanRT
Copy link
Owner

LuanRT commented May 1, 2022

This adds typescript as a development dependency

Nice! That will make things much easier to maintain.
Also, just to let you know there will be a refactor on Actions.js and Request.js to remove redundancy and simplify the way request errors are handled.

lib/core/HomeFeed.js Outdated Show resolved Hide resolved

/**
* @note home_page only returns videos!
* XXX: should some other API be made available to expose the content of the channel
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think advanced users should just use the Simplify API (which I will still document) to get the other types of content on the home page

Maybe the home_page should also expose the channel trailer, but that might break the existing API.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe the home_page should also expose the channel trailer, but that might break the existing API.

The old channel parser was implemented quite recently, so you shouldn't worry about backward compatibility here.

@Wykerd
Copy link
Collaborator Author

Wykerd commented Jun 15, 2022

I don't know if you have noticed, but now the parser traps arrays with a Proxy, that makes it easier to find items etc. Do you think that could completely replace the utilities class Simplify? After some testing I noticed it takes over 2000, 4000ms to find items and maybe the use of regex is to blame.

I've just removed Simplify completely 🎉

@LuanRT
Copy link
Owner

LuanRT commented Jun 15, 2022

I've just removed Simplify completely 🎉

Perfect! Also, could you move all data access classes from /core to /parser/youtube? I'm going to start working on fully supporting YouTube Music and decided it would be more practical to keep their respective classes separated.

@Wykerd
Copy link
Collaborator Author

Wykerd commented Jun 15, 2022

I've just removed Simplify completely 🎉

Perfect! Also, could you move all data access classes from /core to /parser/youtube? I'm going to start working on fully supporting YouTube Music and decided it would be more practical to keep their respective classes separated.

Sure. Some of those classes are very general though (Feed, TabbedFeed, FilterableFeed) and are meant to abstract away from the underlying results structure.

@LuanRT
Copy link
Owner

LuanRT commented Jun 15, 2022

Sure. Some of those classes are very general though (Feed, TabbedFeed, FilterableFeed) and are meant to abstract away from the underlying results structure.

No problem, since those are for core functionality then it should be fine.

@Wykerd
Copy link
Collaborator Author

Wykerd commented Jun 15, 2022

To what degree must I try to maintain backward compatibility with v1?

The new getTrending response is completely different from v1

@LuanRT
Copy link
Owner

LuanRT commented Jun 15, 2022

To what degree must I try to maintain backward compatibility with v1?

The new getTrending response is completely different from v1

Doesn't really matter at this point — we've already changed so many things that it wouldn't make sense to try and keep backward compatibility here and there, but not everywhere.

@Wykerd
Copy link
Collaborator Author

Wykerd commented Jun 15, 2022

I think this PR is about ready to merge then I can continue work on this repo.

@Wykerd Wykerd marked this pull request as ready for review June 15, 2022 21:23
@LuanRT
Copy link
Owner

LuanRT commented Jun 15, 2022

I think this PR is about ready to merge then I can continue work on this repo.

Yup, merging now.

@LuanRT LuanRT merged commit 4a10287 into LuanRT:main Jun 15, 2022
@Wykerd Wykerd mentioned this pull request Jun 15, 2022
4 tasks
@Wykerd Wykerd added this to the v2 milestone Jul 13, 2022
@LuanRT LuanRT added enhancement New feature or request breaking-change labels Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.4] [Sometimes no videos returned]
2 participants