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

getYouTubeVideoID() - support youtube-local pathnames #521

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zrose584
Copy link

@ajayyy
Copy link
Owner

ajayyy commented Nov 21, 2020

Holding off on merging this until I decide how to change the UI to indicate that more than just Invidious instances can be added.

@zrose584
Copy link
Author

zrose584 commented Dec 2, 2020

I think the current MR is already valuable for those who know this feature/functionality is present.
So staling this PR because of UI-Decisions seems a bit unfortunate..

@ajayyy
Copy link
Owner

ajayyy commented Dec 14, 2020

This breaks for YouTube tv URLs:

https://www.youtube.com/tv/#/watch?resume&v=dQw4w9WgXcQ

@@ -801,17 +801,14 @@ function getYouTubeVideoID(url: string) {
}

//Get ID from searchParam
if (urlObject.searchParams.has("v") && ["/watch", "/watch/"].includes(urlObject.pathname) || urlObject.pathname.startsWith("/tv/watch")) {
let m = null;
if (urlObject.searchParams.has("v") && urlObject.pathname.match(/^\/((youtube\.com\/)?watch\/?$|tv\/watch)/)) {
Copy link
Owner

Choose a reason for hiding this comment

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

What is (youtube\.com\/)? for?

Copy link
Author

Choose a reason for hiding this comment

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

youtube-local routes https://youtube.com/watch?v=<vid> to http://your-domain.org/youtube.com/watch?v=<vid>
It does this because it has to route multiple domains, and it probably makes things simpler.

So if youtube-local were to soon also support another video platform (e.g. dailymotion), and they also use /watch?v=, this addon will correctly not transmit the id.
Also note that only ^\/watch would not match youtube-local's routes.

src/content.ts Outdated
}
}
}
else if (m = urlObject.pathname.match(/^\/(?:youtube.com\/)?(?:embed|youtu.be)\/([^/]{11})/)) {
Copy link
Owner

Choose a reason for hiding this comment

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

What is youtu.be for?

Copy link
Author

@zrose584 zrose584 Dec 29, 2020

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

youtu.be should never be a valid URL since it is always redirected

Copy link
Author

@zrose584 zrose584 Dec 30, 2020

Choose a reason for hiding this comment

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

I just tested it, and youtube-local does not redirect the /youtu.be/video_id path to /youtube.com/watch?v=video_id.
It just serves the video as if /youtube.com/watch?v=video_id was used.
Two ways:

  • Merge it as is. The allowance of /youtu.be/video_id is not "hurting" anoyne, right?
  • Remove youtu.be from the regex, and open a ticket on youtube-local. I believe that it should be a quick fix, but I am not 100% sure.. Maybe not being redirected is a feature (idk)?

@ajayyy ajayyy added the question Further information is requested label Dec 14, 2020
@user12345678905
Copy link

Please, complete this pull request, people want block not only sponsor segments!

@war59312
Copy link

If not going to merge these pull requests really should just close them all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants