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: Enabling $page.url store changes when URL is changed directly in address bar #9548

Closed
wants to merge 10 commits into from

Conversation

GabrielBarbosaGV
Copy link

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

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.

@changeset-bot
Copy link

changeset-bot bot commented Mar 28, 2023

🦋 Changeset detected

Latest commit: 185e99f

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

@vicentematus
Copy link
Contributor

vicentematus commented Mar 28, 2023

@GabrielBarbosaGV Does the store.page update correctly inside the hashchange event? I dont know if this is happening to me like i typed on #9374

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?

@GabrielBarbosaGV
Copy link
Author

@vicentematus It appears so - here's the output given by changing the URL (with console.logging every $page.url.hash change)

image

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!

@vicentematus
Copy link
Contributor

vicentematus commented Mar 28, 2023

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

  1. /#7
  2. And then you enter the hash manually lets say /#8 it outputs 7 on the store.

And if you keep going on lets say

  1. /#9 it ouputs 7 (the last state)
  2. /#10 it outpus 9 (last state)

And so on.

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?

The hash change is sufficient because thats the hashchange event emit for. Just for the hash part of the url that changes.

@GabrielBarbosaGV
Copy link
Author

@vicentematus Hello, good day! :D

Would it be like recorded in this GIF? I'm trying to reproduce the issue.

sveltekit_update_hash

@vicentematus
Copy link
Contributor

@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');
		
	}}
/>

@GabrielBarbosaGV
Copy link
Author

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

@vicentematus
Copy link
Contributor

vicentematus commented Mar 29, 2023

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

GabrielBarbosaGV and others added 2 commits April 1, 2023 20:20
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@GabrielBarbosaGV
Copy link
Author

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

@vicentematus
Copy link
Contributor

vicentematus commented Apr 5, 2023

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

@GabrielBarbosaGV
Copy link
Author

@vicentematus Once again, apologies for the wait. I've sent you the invite for becoming a collaborator on my fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$page.url.hash is not updated.
3 participants