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

docs: #15988 follow-up: Use URL.parse to fix Version Switcher URL double slash issue #16014

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

Conversation

NicholasFlamy
Copy link
Member

Description

Uses URL.parse(location.pathname + location.hash, url).href instead of url + location.pathname + location.hash
I noticed the issue after PR #15988 was merged. Sorry about this 3rd PR. Maybe don't merge this until a day after all the comments are addressed so that we don't jinx it. 🤞🤞🤞

How Has This Been Tested?

  • Tested in dev environment with npm run start for docs.

Screenshots (if appropriate)

Before:
image
After:
image

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services)

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 11, 2025
Copy link
Contributor

github-actions bot commented Feb 11, 2025

📖 Documentation deployed to pr-16014.preview.immich.app

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't include any query params.

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 11, 2025

Maybe just to something like location.href.replace(location.origin, url)

@NicholasFlamy
Copy link
Member Author

Maybe just to something like location.href.replace(location.origin, url)

It doesn't handle the double slash, though. So I'll fix the issue in the docs\static\archived-versions.json file, and we have to be sure that doesn't happen again.

@@ -49,7 +49,7 @@ export default function VersionSwitcher(): JSX.Element {
mobile={windowSize === 'mobile'}
items={versions.map(({ label, url }) => ({
label,
to: url + location.pathname + location.hash,
to: URL.parse(location.pathname + location.hash, url).href,
Copy link
Member Author

@NicholasFlamy NicholasFlamy Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to: URL.parse(location.pathname + location.hash, url).href,
to: location.href.replace(location.origin, URL.parse(url.origin)),

If I do this, it handles the double slash issue in case someone forgets about this and it ever happens again.

@bo0tzz
Copy link
Member

bo0tzz commented Feb 11, 2025

The URL constructor won't override the host, but we can just do that explicitly right?
From browser console:

let url = new URL(location.href) 
// URL { href: "https://github.com/immich-app/immich/pull/16014/files", origin: "https://github.com/", protocol: "https:", username: "", password: "", host: "github.com", hostname: "github.com", port: "", pathname: "/immich-app/immich/pull/16014/files", search: "" }
url.host = "immich.app" 
url
// URL { href: "https://immich.app/immich-app/immich/pull/16014/files", origin: "https://immich.app/", protocol: "https:", username: "", password: "", host: "immich.app", hostname: "immich.app", port: "", pathname: "/immich-app/immich/pull/16014/files", search: "" }

@NicholasFlamy
Copy link
Member Author

NicholasFlamy commented Feb 11, 2025

The URL constructor won't override the host, but we can just do that explicitly right? From browser console:

let url = new URL(location.href) 
// URL { href: "https://github.com/immich-app/immich/pull/16014/files", origin: "https://github.com/", protocol: "https:", username: "", password: "", host: "github.com", hostname: "github.com", port: "", pathname: "/immich-app/immich/pull/16014/files", search: "" }
url.host = "immich.app" 
url
// URL { href: "https://immich.app/immich-app/immich/pull/16014/files", origin: "https://immich.app/", protocol: "https:", username: "", password: "", host: "immich.app", hostname: "immich.app", port: "", pathname: "/immich-app/immich/pull/16014/files", search: "" }

Hmm. I'll have to see how that goes.

Edit: in dev builds it'll link to http://immich.app instead of https but that's probably fine since it'll probably redirect to https.

@bo0tzz
Copy link
Member

bo0tzz commented Feb 11, 2025

It should retain the scheme of the original URL, which would be http in dev and https in prod.

@NicholasFlamy
Copy link
Member Author

It should retain the scheme of the original URL, which would be http in dev and https in prod.

It breaks the dev instance though because of the port.

https://url.spec.whatwg.org/#dom-url-host

If the given value for the host setter lacks a port, this’s URL’s port will not change. This can be unexpected as host getter does return a URL-port string so one might have assumed the setter to always "reset" both.

@NicholasFlamy NicholasFlamy force-pushed the docs-version-switcher-use-current-path branch from 2353b6e to b6d364d Compare February 12, 2025 03:46
@NicholasFlamy NicholasFlamy force-pushed the docs-version-switcher-use-current-path branch from b6d364d to 09f81c5 Compare February 12, 2025 03:50
@NicholasFlamy
Copy link
Member Author

Let me know what you think of the latest implementation. There doesn't seem to be any easy and obvious way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:skip documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants