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

fix(routing): fix history routerMode #2399

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

Conversation

Beat-YT
Copy link
Contributor

@Beat-YT Beat-YT commented Jul 7, 2024

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.

@jellyfin-bot
Copy link

jellyfin-bot commented Jul 7, 2024

Cloudflare Pages deployment

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

View bot logs

@Beat-YT Beat-YT changed the title fix(serverAdd): block server addition & fix history routerMode fix(routing): block server addition & fix history routerMode Jul 7, 2024
@ferferga
Copy link
Member

ferferga commented Jul 7, 2024

@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 #2108 and #2085?

@Beat-YT
Copy link
Contributor Author

Beat-YT commented Jul 7, 2024

@ferferga I believe this overcomes the situations described in #2108 and #2085

@Beat-YT
Copy link
Contributor Author

Beat-YT commented Jul 8, 2024

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

@ferferga
Copy link
Member

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

@Beat-YT
Copy link
Contributor Author

Beat-YT commented Jul 11, 2024

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

Copy link

sonarcloud bot commented Jul 29, 2024

@Beat-YT Beat-YT changed the title fix(routing): block server addition & fix history routerMode fix(routing): fix history routerMode Jul 29, 2024
@Beat-YT
Copy link
Contributor Author

Beat-YT commented Jul 29, 2024

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

@ferferga
Copy link
Member

ferferga commented Aug 9, 2024

@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
Copy link
Contributor Author

Beat-YT commented Aug 9, 2024

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.

@ferferga ferferga force-pushed the master branch 13 times, most recently from aad87ab to a2cdbb4 Compare August 11, 2024 10:49
@ferferga ferferga force-pushed the master branch 2 times, most recently from acb6ecf to 5d6a7c8 Compare August 11, 2024 11:13
@ferferga ferferga marked this pull request as draft August 17, 2024 10:47
@jellyfin-bot jellyfin-bot added the merge conflict Something has merge conflicts label Sep 6, 2024
@ferferga ferferga force-pushed the master branch 2 times, most recently from c360429 to f3260d9 Compare September 6, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Something has merge conflicts
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants