-
-
Notifications
You must be signed in to change notification settings - Fork 336
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
base: master
Are you sure you want to change the base?
Conversation
Holding off on merging this until I decide how to change the UI to indicate that more than just Invidious instances can be added. |
I think the current MR is already valuable for those who know this feature/functionality is present. |
This breaks for YouTube tv URLs:
|
@@ -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)/)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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})/)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #521 (comment)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)?
…support_ytlocal # Conflicts: # src/content.ts
Please, complete this pull request, people want block not only sponsor segments! |
If not going to merge these pull requests really should just close them all. |
user234683/youtube-local#30