jellyfin-vue icon indicating copy to clipboard operation
jellyfin-vue copied to clipboard

fix(routing): fix history routerMode

Open Beat-YT opened this issue 1 year ago β€’ 10 comments

Fixes

  • serverAdd: Block server addition and selection when disallowed by config.
  • router: Correct history routerMode to ensure proper asset pathing.

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

  • Verified that server addition and selection are blocked when allowServerSelection is false.
  • Tested history routerMode functionality to ensure static assets are correctly pathed.

Beat-YT avatar Jul 07 '24 01:07 Beat-YT

Cloudflare Pages deployment

Latest commit c36d373
Status πŸ”„ Deploying...
Preview URL Not available
Type πŸ”€ Preview

View bot logs

jellyfin-bot avatar Jul 07 '24 01:07 jellyfin-bot

@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?

ferferga avatar Jul 07 '24 08:07 ferferga

@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 avatar Jul 07 '24 09:07 Beat-YT

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

Beat-YT avatar Jul 08 '24 00:07 Beat-YT

@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:

  • Served from the root path
  • Served from subpath

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:

  • Handle it themselves at the webserver level
  • Build the client with the desired base.

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.

ferferga avatar Jul 11 '24 15:07 ferferga

@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.

Beat-YT avatar Jul 11 '24 17:07 Beat-YT

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

Beat-YT avatar Jul 29 '24 04:07 Beat-YT

@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.

ferferga avatar Aug 09 '24 14:08 ferferga

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.

Beat-YT avatar Aug 09 '24 15:08 Beat-YT

Hello @Beat-YT, I'm going to close this in the meantime for tidying up tha backlog a little. If you ever are interested in tackling this again, feel free to open a new one.

ferferga avatar Nov 29 '24 12:11 ferferga