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: Add Shorts endpoint #512

Merged
merged 12 commits into from
Dec 1, 2023
Merged

feat: Add Shorts endpoint #512

merged 12 commits into from
Dec 1, 2023

Conversation

Duell10111
Copy link
Contributor

This PR should start integrating the short endpoints as discussed here:
#482

As I still do not have the overview over the library, the changes maybe do not fit to the library still.
Any help is welcome. :)

Copy link
Collaborator

@absidue absidue left a comment

Choose a reason for hiding this comment

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

In general the pull request looks good, haven't tested it, just a few nits around the code style, performance and not parsing tracking information.

I'll let Luan decide on the naming of things, e.g. the watch route not fully matching the path name and a few mentions of renderer in properties, which YouTube.js tries to stay away from, unless there is already a field that has it without the renderer in it's name.

src/Innertube.ts Outdated
@@ -39,6 +41,7 @@ import type { ApiResponse } from './core/Actions.js';
import { type IBrowseResponse, type IParsedResponse } from './parser/types/index.js';
import type { INextRequest } from './types/index.js';
import type { DownloadOptions, FormatOptions } from './types/FormatUtils.js';
import {encodeReelSequence} from './proto/index.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add spaces inside the brackets to match the code style used for the other imports, please address it in the other files too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adapted and added a eslint rule to create warnings about this issue

src/Innertube.ts Outdated Show resolved Hide resolved
src/core/endpoints/reel/WatchEndpoint.ts Outdated Show resolved Hide resolved
src/core/endpoints/reel/WatchSequenceEndpoint.ts Outdated Show resolved Hide resolved
src/parser/classes/Command.ts Outdated Show resolved Hide resolved
src/parser/classes/Command.ts Outdated Show resolved Hide resolved
src/parser/continuations.ts Outdated Show resolved Hide resolved
src/types/Endpoints.ts Outdated Show resolved Hide resolved
src/parser/classes/ReelPlayerOverlay.ts Outdated Show resolved Hide resolved
@Duell10111 Duell10111 marked this pull request as ready for review September 30, 2023 19:24
test/main.test.ts Outdated Show resolved Hide resolved
@Duell10111
Copy link
Contributor Author

Are there any other recommendations left regarding this PR. 😅

@fabcotech
Copy link

+1 I'd very much to get shorts as well, thanks for the work

Copy link
Collaborator

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Please fix the linting warnings triggered by this pull request (added code suggestions in the relevant places).

src/parser/classes/ReelPlayerHeader.ts Outdated Show resolved Hide resolved
src/core/Actions.ts Outdated Show resolved Hide resolved
@Duell10111
Copy link
Contributor Author

Please fix the linting warnings triggered by this pull request (added code suggestions in the relevant places).

Fixed eslint erros as you suggested and rebased PR. :)

Copy link
Collaborator

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Please used named imports for the Parser instead of a default one, please see #535 for why. You only need to change the files you added, the linked pull request changes all other ones.

src/parser/classes/ReelPlayerOverlay.ts Outdated Show resolved Hide resolved
src/parser/ytshorts/VideoInfo.ts Outdated Show resolved Hide resolved
Duell10111 and others added 2 commits November 2, 2023 23:26
Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
Co-authored-by: absidue <48293849+absidue@users.noreply.github.com>
@LuanRT LuanRT changed the title chore: add short endpoint feat: Add Shorts endpoint Dec 1, 2023
@LuanRT LuanRT merged commit a32aa8c into LuanRT:main Dec 1, 2023
4 checks passed
absidue added a commit to absidue/YouTube.js that referenced this pull request Dec 1, 2023
absidue added a commit to absidue/YouTube.js that referenced this pull request Dec 1, 2023
Author: Konstantin <duell10111@t-online.de>
absidue added a commit to absidue/YouTube.js that referenced this pull request Dec 1, 2023
Authored-by: Konstantin <duell10111@t-online.de>
absidue pushed a commit to absidue/YouTube.js that referenced this pull request Dec 1, 2023
LuanRT pushed a commit that referenced this pull request Dec 2, 2023
Co-authored-by: Konstantin <duell10111@t-online.de>
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.

None yet

5 participants