-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: Enabling $page.url store changes when URL is changed directly in address bar #9548
Conversation
🦋 Changeset detectedLatest commit: 185e99f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@GabrielBarbosaGV Does the Are you checking if whenever you change the hash manually on the search bar, it console log the correct store value on the hashchange event? |
@vicentematus It appears so - here's the output given by changing the URL (with console.logging every $page.url.hash change) On the other hand, when trying to log $page.url directly, no changes are displayed. This is something I hadn't noticed, and I am unsure as to what could be causing it. Is it troublesome, or is the hash change being detected sufficient? Many thanks! |
@GabrielBarbosaGV It's working weird: its correctly changing when you click on anchor elements, and then update the hash manually on the search bar. But if you enter the manually the hash before all it doesnt update well: If you first enter a page with the url and a hash lets say you enter the following page:
And if you keep going on lets say
And so on.
The hash change is sufficient because thats the |
@vicentematus Hello, good day! :D Would it be like recorded in this GIF? I'm trying to reproduce the issue. |
@GabrielBarbosaGV hey, good morning too. Use this code on a +page.svelte to debug this: <script>
import { page } from '$app/stores';
$: console.log('Store', $page.url.hash);
</script>
<svelte:window
on:hashchange={() => {
console.log($page.url.hash, 'store');
console.log(window.location.hash, 'location');
}}
/>
|
@vicentematus I was able to reproduce what you said. I then attempted this: <script>
import { page } from '$app/stores';
</script>
<div id="store-data">{JSON.stringify($page.data)}</div>
<div id="store-error">{$page.error?.message}</div>
<div id="page-url">{$page.url}</div>
<svelte:window on:hashchange={() => setTimeout(() => console.log($page.url.hash), 1000)} />
<nav>
<a href="/store/data/xxx">xxx</a> <a href="/store/data/yyy">yyy</a>
<a href="/store/data/zzz">zzz</a> <a href="/store/data/foo">foo</a>
</nav>
<slot /> and, with the timeout, the correct number is displayed. The problem here seems to be that the emission of the hashchange event is calling both the svelte:window callback and the one that updates the store, but the earlier beats the latter to the punch. I confess I am unsure of how to approach this - I know there is a part of the code that ensures the hashchange event is fired even with direct address bar changes, and, perhaps, moving the store update from where it currently is to over there could be a solution, but I don't feel like it's the right approach, at least in my gut. What do you think? |
@GabrielBarbosaGV so it was some weird race condition that is emitting one event after the other. There are 2 events that are emitted (from what i experimented): popstate and hashchange. You can find them both in the same client.js file. I think the popstate is executing first and then the hashchange one. Like Javascript is single threaded the one that execute first wins. Also maybe there's another event that we are missing out? I can't recall. If you try to debug the popstate eventlistener you can check the following somewhat not related. Maybe is because both event listeners at the same time. |
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@vicentematus I apologise for the wait, I thought I'd written here already. Since we last spoke, I've tried moving the code to popstate, but the problem remains. I must confess that, as of now, I do not know of a solution to the issue. Another problem is the test that should fail without the fix, but work with it. |
@GabrielBarbosaGV it's up to the maintainers if we should ignore it and just open another issue about that problem. Just to speed up things up. Can you give me access to your sveltejs kit fork? |
@vicentematus Once again, apologies for the wait. I've sent you the invite for becoming a collaborator on my fork. |
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Fixes #9374
Whilst the hashchange event is already fire when the URL is altered directly in the address bar, the $page.url store would not be updated. This PR enables the expected change.