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.

Sign up for GitHub

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

Jump to bottom

fix: correctly detect internal links when hash router is used #13296

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: correctly detect internal links when hash router is used #13296

wants to merge 3 commits into from
+8 −3

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Jan 9, 2025 •
edited
Loading

fixes #13287

This PR fixes the client-side external link evaluation so that when using the hash router, accessing /index.html is considered an internal link. This prevents the page from reloading infinitely because the router incorrectly considered /index.html an external link.

There's also a slight doc change. Instead of suggesting all links start with /#/, suggesting links start with # allows the router to preserve the /index.html pathname. For example, links to /about should be #/about instead of /#/about so that it navigates to /index.html#/about.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 9, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 2abef82

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@eltigerchino eltigerchino changed the title fix and changeset fix: treat /index.html as an internal link when hash router is used Jan 9, 2025
@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-kit-13296-svelte.vercel.app/

this is an automated message

@eltigerchino eltigerchino changed the title fix: treat /index.html as an internal link when hash router is used fix: treat /index.html as an internal link when the hash router is used Jan 9, 2025
@eltigerchino eltigerchino marked this pull request as draft January 9, 2025 11:48
@eltigerchino eltigerchino changed the title fix: treat /index.html as an internal link when the hash router is used fix: correctly detect internal links when hash router is used Jan 9, 2025
@eltigerchino
Copy link
Member Author

eltigerchino commented Jan 9, 2025 •
edited
Loading

Seems to still infinitely reload if we embed the app in an iframe using srcdoc because the URL becomes about:srcdoc and the pathname is srcdoc.

EDIT: I think we'll just deal with the embedded app html text case in a different PR as there are more compatibility issues to deal with first #13226 . Embedding through a URL src seems to work fine.

@eltigerchino eltigerchino marked this pull request as ready for review January 10, 2025 07:11
@sateeshsai
Copy link

sateeshsai commented Jan 10, 2025 •
edited
Loading

There's also a slight doc change. Instead of suggesting all links start with /#/, suggesting links start with # allows the router to preserve the /index.html pathname. For example, links to /about should be #/about instead of /#/about so that it navigates to /index.html#about.

This is great. I was going to raise an issue for this. '.../index.html/#/about' causes browser navigation in pages served by certain web servers.

Would this change work for urls that have the page name other than /index.html? For example, .../somepage.html

@stephenlrandall

This comment was marked as off-topic.

Sign in to view
@eltigerchino
Copy link
Member Author

Would this change work for urls that have the page name other than /index.html? For example, .../somepage.html

No, because the hash router build currently always generates an index.html page as the root page.

Wouldn't /index.html#about be expected to land on an element with id about on the /index.html page?

Yes, but you don't want to write all your links as href="/index.html#about" so you can just write href="#about" for the same effect.

@stephenlrandall
Copy link
Contributor

No, because the hash router build currently always generates an index.html page as the root page.

This actually isn't documented at the moment, and it's not clear from the documentation how this interacts with the adapter-static fallback page option.

Wouldn't /index.html#about be expected to land on an element with id about on the /index.html page?

Yes, but you don't want to write all your links as href="/index.html#about" so you can just write href="#about" for the same effect.

So in the event someone has an "About this project" section on their home page with id="about" but also a page called "About" at /about, how would you link to each? Should href="#about" take you down to the section or over to the other page? Would something like href="#/about#authors" correctly take you to the "Authors" section over on /about? I apologize for all the questions, but it's not clear from the documentation how this is supposed to work.

@eltigerchino
Copy link
Member Author

Wouldn't /index.html#about be expected to land on an element with id about on the /index.html page?

Yes, but you don't want to write all your links as href="/index.html#about" so you can just write href="#about" for the same effect.

So in the event someone has an "About this project" section on their home page with id="about" but also a page called "About" at /about, how would you link to each? Should href="#about" take you down to the section or over to the other page?

Sorry, I'm mistaken. The link should be href="#/about". The slash after the hash will cause it to navigate to a different page.

Would something like href="#/about#authors" correctly take you to the "Authors" section over on /about? I apologize for all the questions, but it's not clear from the documentation how this is supposed to work.

Yes, that will take you to authors section in the about page. Perhaps you can create a new issue and outline the hash router mechanics that require clarification in the documentation.

@stephenlrandall stephenlrandall mentioned this pull request Jan 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@eltigerchino @Rich-Harris @sateeshsai @stephenlrandall