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

Script does not play nicely with Umbrel instance #1

Closed
anthony-robin opened this issue May 3, 2024 · 7 comments · Fixed by #2 or #3
Closed

Script does not play nicely with Umbrel instance #1

anthony-robin opened this issue May 3, 2024 · 7 comments · Fixed by #2 or #3
Assignees
Labels
enhancement New feature or request

Comments

@anthony-robin
Copy link

Thank you very much for this script, this feature is so nice to have on Invidious ! ❤️

I tried it with my Umbrel Invidious self-hosted app and I had to make some adjustements to make it work. After investigating the script, I found that const IVinstance = "https://" + window.location.hostname;:

  • assumes https (Umbrel does not yet support it)
  • does not forward custom ports (Umbrel uses custom port)

Would you consider supporting out of the box Umbrel instance for these default URLs:

  • http://umbrel.local:3420 (from same network as Umbrel)
  • http://umbrel:3420 (from external network using Tailscale VPN)

or, maybe adapt IVinstance to not force https but allowing custom port, then letting users to manually add any Invidious self-hosted instance in Tampermonkey settings ?

What do you think about this ?
Thank you :)

@WalsGit
Copy link
Owner

WalsGit commented May 3, 2024

Hi @anthony-robin!
First, thank you for message and feedback.

As for this issue, if I'm not mistaken anyone could add an instance URL to match in Tampermonkey : in the dashboard, click on the script then on the settings tab, and in the include/exclude section, click on the add button under the Users Matches field.

As you can understand, since there's an undefined number of IV instances, It's impossible to match all of them right out of the box in the script. That's why I decided to only match the main ones listed on their api website (the https ones to be precise as I don't know how to deal with the other protocoles).

However, although I'm not familiar with Umbrel, I could add the URLs if by default they are the same (with the same port) for all users who install and use IV through it (and for any other OS that has a common default local IV url).

Finally, we are left with the https problem which I think I could also fix.

@WalsGit WalsGit self-assigned this May 3, 2024
@WalsGit WalsGit linked a pull request May 3, 2024 that will close this issue
@WalsGit WalsGit closed this as completed in #2 May 3, 2024
@WalsGit WalsGit reopened this May 3, 2024
@WalsGit
Copy link
Owner

WalsGit commented May 3, 2024

I added support for http protocol.

however for Umbrel, I'm waiting for you to confirm to me that IV always (or by default at least) uses the port 3420, as I'm looking into how I could make it work.

@WalsGit WalsGit added the enhancement New feature or request label May 3, 2024
@anthony-robin
Copy link
Author

Thank you very much for your quick reply and for considering my issue :)

I can confirm you that everybody installing Invidious from Umbrel would have default port to 3420 (it is hardcoded in app store metadata and docker-compose).

I think it is not a big deal to have to add manually these URL in Tampermonkey settings as long as const IVinstance= take cares of the port and the hostname.

Thank you :)

@WalsGit
Copy link
Owner

WalsGit commented May 6, 2024

Hi,
It seems like the @match directive ignores the ports in matching the domain and and I found out that I had to use @includes to target a domain with a specific port. here is a new version of the script, would you please test it with your setup and give me feedback before I publish it? https://github.com/WalsGit/Add2WL-for-Invidious/blob/umbrel-support/Add2WL-for-Invidious.user.js

@anthony-robin
Copy link
Author

Your script did not work on my Umbrel instance, port did not get append to the host. Why not using window.location.host instead of window.location.hostname in const IVInstance=, that way you will get the port taken in consideration ?
See https://stackoverflow.com/a/6042031/3700317 for more details.

Capture d’écran 2024-05-06 à 16 39 30

It seems also that @include is not recomended anymore :(
Capture d’écran 2024-05-06 à 16 40 37

After updating the code as following, script worked again:

// @match          *://umbrel.local:3420/*
// @match          *://umbrel:3420/*

// ...
const IVinstance = protocol + "//" + window.location.host;
// ...

First @match is Umbrel domain on same local network and the second one is using Tailscale VPN to access Umbrel from outside the local network. window.location.host keep the port.

What do you think about this ? :)

@WalsGit
Copy link
Owner

WalsGit commented May 6, 2024

oh I didn't know about window.location.host (and its difference with hostname). I'll change that. that should fix everything.

thank you for your help!

@anthony-robin
Copy link
Author

You're welcome :)

@WalsGit WalsGit linked a pull request May 6, 2024 that will close this issue
@WalsGit WalsGit closed this as completed in #3 May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants