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

API: Add playlist and start time to resolve_url #4205

Merged
merged 5 commits into from
Feb 12, 2024

Conversation

iBicha
Copy link
Contributor

@iBicha iBicha commented Oct 26, 2023

Captures the start time and the playlist id (if any) of the resolved url.

Also, make a head request to check for redirects. This is to resolve short urls (https://youtu.be/<videoId>?t=60)

@iBicha iBicha requested a review from a team as a code owner October 26, 2023 21:25
@iBicha iBicha requested review from SamantazFox and removed request for a team October 26, 2023 21:25
Copy link
Member

@syeopite syeopite left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I'm not entirely sure about the additional redirect check though.

The resolve url endpoint in Invidious is mostly used as a limited proxy to InnerTube's resolve url. As the original doesn't support youtu.be should Invidious even tact on that extra functionality?

@iBicha
Copy link
Contributor Author

iBicha commented Nov 8, 2023

Code looks good to me. I'm not entirely sure about the additional redirect check though.

The resolve url endpoint in Invidious is mostly used as a limited proxy to InnerTube's resolve url. As the original doesn't support youtu.be should Invidious even tact on that extra functionality?

I agree with you, the main reason I did add it is because a web client wouldn't be able to do that redirect check because of CORS.

Because it's an endpoint that is not often used, it felt that adding the HEAD call wouldn't be a big deal, and adds the benefit of supporting short URLs and web clients.
If there's a strong opinion against it, I'm okay with removing it

@absidue
Copy link
Contributor

absidue commented Nov 10, 2023

It sounds like your use case is passing every URL to Invidious to pass to YouTube to resolve, personally I feel like that is the wrong approach. Parsing YouTube URLs is not that hard to do, especially youtu.be ones, also means your app will be faster, because you aren't making unnecessary network requests.

Your reasoning for web clients goes out the window too, if you add parsing code.

The only use case, where you have to use the resolve endpoint, is resolving vanity channel URLs (https://youtube.com/@YouTube, https://youtube.com/user/YouTube, https://youtube.com/c/YouTube and https://youtube.com/YouTube) to get the channel ID, as the channel APIs (Invidious' and Innertube/YouTube) need the channel ID.

@SamantazFox
Copy link
Member

I second Absidue's comment here: adding support for youtu.be is not useful, as it's a self-explanatory URL (everything you need is already there).

As for the rest, are there cases where the playlist ID or the video's start time are hidden in a shortened URL?

@iBicha
Copy link
Contributor Author

iBicha commented Nov 15, 2023

What's the recommendation? Should I remove the redirect? Or close the PR?

@iBicha
Copy link
Contributor Author

iBicha commented Nov 15, 2023

I second Absidue's comment here: adding support for youtu.be is not useful, as it's a self-explanatory URL (everything you need is already there).

As for the rest, are there cases where the playlist ID or the video's start time are hidden in a shortened URL?

I added this for completeness on both fronts, the resolveUrl api and the short urls. Youtube introduces things all the time (mixes, shorts, etc) and it's nice to have an endpoint you can rely on to tell you what things are.

Also, if the resolveUrl is returning data (like playlistId or startTimeSeconds) then Invidious should return it

@syeopite
Copy link
Member

What's the recommendation? Should I remove the redirect? Or close the PR?

I'll say just remove the redirect. The rest of the changes are fine.

As for the rest, are there cases where the playlist ID or the video's start time are hidden in a shortened URL?

While not hidden in a shortened url the resolve endpoint does return those attributes in a standard watch?v= link like

https://www.youtube.com/watch?v=n3Xv_g3g-mA&list=PLFs4vir_WsTySi9F8v5pvCi6zQj7Cwneu&t=60
resolve

@iBicha
Copy link
Contributor Author

iBicha commented Nov 19, 2023

Looks like there's a consensus on not doing the HEAD call. It's removed

@SamantazFox SamantazFox added the need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something label Nov 28, 2023
@SamantazFox SamantazFox added ready and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Jan 14, 2024
@SamantazFox SamantazFox changed the title Add playlist and start time to the resolve url API: Add playlist and start time to resolve_url Feb 12, 2024
@SamantazFox SamantazFox merged commit 9bd2072 into iv-org:master Feb 12, 2024
@iBicha iBicha deleted the feature/resolve-url-fields branch February 12, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants