Fix client side router issue with handling hash

[PH4NTOMiki]

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.

fixes #3746 issue

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: b53bbbd

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

[netlify]

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: b53bbbd

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/6201616953adad0008277f05

[ignatiusmb]

LGTM, I've added a couple of suggestions

[PH4NTOMiki]

Thank you @ignatiusmb for the review, committed

[benmccann]

wtf. why do browsers behave this way?! I never would have expected removing a hash to result in a full page navigation. I guess we missed that when implementing #3177

I would expect the hashchange event to be fired if you remove the hash, but it's not with this PR. But I suppose that's okay since I'm assuming browsers won't do it since they issue a full navigation - I haven't tested this though and browsers thus far have defied my expectations. What do you think @bluwy?

[PH4NTOMiki]

yeah, I agree @benmccann but the funny thing is that from experience(a vanilla web app I developed a long time ago) I know that it doesn't, I actually use history.pushState specifically(and only) to remove the hash part, and I never had issues with it, so there must be some other player at scene. I'm not sure what tho, but as I wrote in #3746 I think this is a bigger problem than originally thought. I'll try some more fiddling with this. And I'll write anything I found in the #3746 issue.

[bluwy]

I would expect the hashchange event to be fired if you remove the hash, but it's not with this PR. But I suppose that's okay since I'm assuming browsers won't do it since they issue a full navigation - I haven't tested this though and browsers thus far have defied my expectations. What do you think @bluwy?

Yeah I think this PR's behaviour is correct. It looks like hashchange is only called when you navigate to another hash, so it's actually my bad on #3177 😬 Would be better to be closer to the standards.

[benmccann]

I expanded the code comments since this section of code is a bit bug-prone and will go ahead and merge this

Thanks for the fix!!

[PH4NTOMiki]

@bluwy @ignatiusmb Also i'm not sure about the /page1# specific use case, should that be treated by that conditional block or not(do we consider it to fall in that category or not), my first commit was just checking for the splitted array length to be longer than 1, so that meant that the condition was triggered even if the hash was just '#' but I'm not sure, but the rewrite @ignatiusmb suggested and I committed changed that, because the truthiness of empty string is false, so we're skiping that conditional block.
So which is correct to add that.

And @benmccann to clarify myself, on vanilla web app, if you do history.pushState({}, '', location.href.split('#')[0]) it would do nothing, but if you do location.hash = '' it would trigger a full page reload.

[PH4NTOMiki]

@benmccann I think we should have waited to clarify this (the '/page1#' edge-case) before commit, I was just writing previous comment when you merged it., I'll push a PR if needed to fix that if needed

[benmccann]

I think /page1# should go into the if block based on the browser's behavior