search

jellyfin / jellyfin-vue

A modern web client for Jellyfin based on Vue
https://jellyfin.org
GNU General Public License v3.0
1.24k stars 223 forks source link

fix(routing): fix history routerMode #2399

Open Beat-YT opened 2 months ago

Beat-YT commented 2 months ago

Fixes

Description

  1. Updated the server addition and selection logic to respect the allowServerSelection configuration setting.
  2. Fixed an issue with the history routerMode where the base path was incorrectly set to ./, causing static assets to be pathed incorrectly (e.g., /server/assets/example.png instead of /assets/example.png when on /server/login).

Testing

jellyfin-bot commented 2 months ago

Cloudflare Pages deployment

Latest commit c36d373
Status 🔄 Deploying...
Preview URL Not available
Type 🔀 Preview

View bot logs

ferferga commented 2 months ago

@Beat-YT Thank you very much for your PR! Have you checked if the revert to absolute paths doesn't cause a regression in the situations described in https://github.com/jellyfin/jellyfin-vue/pull/2108 and https://github.com/jellyfin/jellyfin-vue/issues/2085?

Beat-YT commented 2 months ago

@ferferga I believe this overcomes the situations described in https://github.com/jellyfin/jellyfin-vue/pull/2108 and https://github.com/jellyfin/jellyfin-vue/issues/2085

Beat-YT commented 2 months ago

@ferferga an alternative solution would be to race promise between the absolute and relative config file.

ferferga commented 1 month ago

@Beat-YT I've tested this and this makes a regression to both issues. This is a problem that we've been dealing with for some time and I researched it extensively: the issue when using HTML5 history mode is that the browser can't distinguish between what's a route inside the application and what's the server path. For example if I'm in /settings, what does settings mean, it's inside the app or the webserver? That's the distinction that the shebang hints at the web browser. I thought at first that import.meta.env was something I missed back in the day where the browser provides info about the environment, but that just prints whatever base property it's set at build time, so this is still impossible to achieve at runtime.

We need the frontend to be as compatible as possible out of the box and currently it is, so it can be hosted out of the box when:

Sadly that means we need the hash history but, believe me, I'd also like to get rid of it. It would be awesome for everybody if we could get rid of any hardcoded base option and have it calculated at runtime. But currently it's impossible. For someone who want clean urls, the solution is:

You can quickly reproduce all of this by spinning a Codespace in your branch, replace location / { from here to location /subpath1/subpath2 { and this one to COPY --from=build /app/frontend/dist/ /usr/share/nginx/html/subpath1/subpath2 and build the Docker image.

As for the other commit, it seems fine to me, so that specific one can be cherrypicked. I'll wait for your feedback first, in case I misunderstood something of your implementation and then we can follow with that commit.

Beat-YT commented 1 month ago

@ferferga Thanks for your reply!

Perhaps using a plugin such as vite-plugin-dynamic-base can do the trick?

I can PoC the implementation if you are fine with adding said plugin to the project.

Otherwise I agree with you that it’s better to keep using the current hash router.

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Beat-YT commented 1 month ago

I removed the block server addition part to open a new pull request about it.

ferferga commented 4 weeks ago

@Beat-YT The plugin won't fix anything either (at least unless I'm missing something you saw but I didn't).

What Vite does is hardcode whatever string is given to the base option so it's used everywhere. What the plugin does is to replace that hardocoded string to a JS variable, so you can change that variable at runtime using JS. But the plugin doesn't "magically" solves the problematic of identifying which part is which from the URL.

Beat-YT commented 4 weeks ago

The logic to determine the base path is up to us to make.

I was thinking we could add and ENV to the docker to set the base path accordingly, much like the config is being edited at runtime via the environment variables.