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

page.url not reactive on the new $app/state module. #13187

Closed
Gonzalo9823 opened this issue Dec 17, 2024 · 11 comments · Fixed by #13196
Closed

page.url not reactive on the new $app/state module. #13187

Gonzalo9823 opened this issue Dec 17, 2024 · 11 comments · Fixed by #13196
Labels
bug Something isn't working

Comments

@Gonzalo9823
Copy link

Describe the bug

When you try to hear for changes on page.url using $derived it won't work. For example If ones has a +page.svelte that wants the value of a searchParams like this:

const currentPage = $derived(page.url.searchParams.get("page") || "");

And then you update the searchParams using goto:

const { url } = page;

url.searchParams.set('page', newPage);

goto(url, { replaceState: true, noScroll: true, keepFocus: true, invalidateAll: true })

The value of currentPage won't be updated it. I tried multiple combinations of this, and the only thing that works is listening to another value of page. For example something like this:

const currentPage = $derived.by(() => {
  page.state;

  return page.url.searchParams.get('page') || '';
});

Now if I change the searchParams my currentPage will be updated to the correct value.

Also this used to work using the $app/store module:

// This works.
const currentPage = $derived($page.url.searchParams.get('page') || '');

Reproduction

https://replit.com/@GonzaloCaballe1/Reproduction

+page.svelte:

<script>
  import { page } from "$app/state";
  import { goto } from "$app/navigation";

  const currentPage = $derived(page.url.searchParams.get("page") || "");

  const currentPageWorking = $derived.by(() => {
    page.state;
    return page.url.searchParams.get("page") || "";
  });
</script>

<div>
  <label for="current_page">Current Page:</label>
  <input
    id="current_page"
    bind:value={
      () => page.url.searchParams.get("page"),
      (newPage) => {
        const { url } = page;

        url.searchParams.set("page", newPage);

        goto(url, {
          replaceState: true,
          noScroll: true,
          keepFocus: true,
          invalidateAll: true,
        });
      }
    }
  />
</div>

<h1>Current Page Not Working: {currentPage}</h1>
<h1>Current Page Working: {currentPageWorking}</h1>

Logs

No response

System Info

System:
  OS: macOS 14.5
  CPU: (10) arm64 Apple M1 Pro
  Memory: 105.55 MB / 16.00 GB
  Shell: 5.9 - /bin/zsh
Binaries:
  Node: 22.11.0 - ~/.nvm/versions/node/v22.11.0/bin/node
  npm: 10.9.0 - ~/.nvm/versions/node/v22.11.0/bin/npm
  pnpm: 9.14.2 - ~/Library/pnpm/pnpm
  bun: 1.1.9 - ~/.bun/bin/bun
npmPackages:
  svelte: 5.14.1 => 5.14.1

Severity

annoyance

@Conduitry Conduitry transferred this issue from sveltejs/svelte Dec 17, 2024
@Leonidaz
Copy link

according to the docs goto takes a string or URL. However, this only currently works with a string.

Working Reproduction SvelteLab

this works or url.toString()

        goto(`?${url.searchParams.toString()}`, {
          replaceState: true,
          noScroll: true,
          keepFocus: true,
          invalidateAll: true,
        });

this doesn't:

        goto(url, {
          replaceState: true,
          noScroll: true,
          keepFocus: true,
          invalidateAll: true,
        });

Svelte Reactivity seems to be fine with SvelteURL

@eltigerchino eltigerchino added bug Something isn't working and removed bug Something isn't working labels Dec 18, 2024
@eltigerchino
Copy link
Member

eltigerchino commented Dec 18, 2024

The snippet below with a new URLSearchParams object works because we're not mutating the $page/state URL object

<script>
  import { page } from "$app/state";
  import { goto } from "$app/navigation";

  const currentPage = $derived(page.url.searchParams.get("page"));
</script>

<div>
  <label for="current_page">Current Page:</label>
  <input
    id="current_page"
    bind:value={() => page.url.searchParams.get("page"),
    (value) => {
      const searchParams = new URLSearchParams(page.url.search);

      if (value) {
        searchParams.set("page", value);
      } else {
        searchParams.delete("page");
      }

      goto(`?${searchParams}`, {
        replaceState: true,
        noScroll: true,
        keepFocus: true,
        invalidateAll: true,
      });
    }}
  />
</div>

<h1>Current Page: {currentPage}</h1>

or creating a new URL object

<script>
  import { page } from "$app/state";
  import { goto } from "$app/navigation";

  const currentPage = $derived(page.url.searchParams.get("page"));
</script>

<div>
  <label for="current_page">Current Page:</label>
  <input
    id="current_page"
    bind:value={() => page.url.searchParams.get("page"),
    (value) => {
      const url = new URL(page.url);

      if (value) {
        url.searchParams.set("page", value);
      } else {
        url.searchParams.delete("page");
      }

      goto(url, {
        replaceState: true,
        noScroll: true,
        keepFocus: true,
        invalidateAll: true,
      });
    }}
  />
</div>

<h1>Current Page: {currentPage}</h1>

As an aside, it's kind of a similar request to #13178 where people want $page/state to be mutable whereas I think it wasn't intended to work this way

EDIT: actually, it is reactive but it's just not working with the proxied URL. Doing goto(new URL(url)) seems to work too

@eltigerchino eltigerchino added the bug Something isn't working label Dec 18, 2024
@dummdidumm
Copy link
Member

This is probably because when the URL is not actually updating by reference, $state.raw says "nothing changed" and won't trigger an update.
Either we always wrap it in a new URL(...) or we implement a version counter under the hood for it.

@Leonidaz
Copy link

@dummdidumm @Rich-Harris was the page.url created as state.raw on purpose so it's not deeply reactive with unwanted side-effects?

why not use SvelteURL for full reactivity? e.g. playground SvelteURL with searchParams reactivity

url = $state.raw(new URL('https://example.com'));

@dummdidumm
Copy link
Member

while that's a nice idea, we can't do that at least in SvelteKit 2 while we still support Svelte 4, because we would need to import it from somewhere that does not exist in Svelte 4.

@Leonidaz
Copy link

while that's a nice idea, we can't do that at least in SvelteKit 2 while we still support Svelte 4, because we would need to import it from somewhere that does not exist in Svelte 4.

but how are we using $state.raw then? There is a flag above to check for legacy: https://github.com/sveltejs/kit/blob/089159cc10a6d5a00cb693b48fc543785ca23e6b/packages/kit/src/runtime/client/state.svelte.js#L17C5-L40

But if you mean that the behavior in SvelteKit 2 with Svelte v4 vs v5 will be different (reactive vs not), yeah.... I mean, it could be documented and people with Svelte 5 can start benefitting from this without having to wait for SvelteKit version 3.

@Rich-Harris
Copy link
Member

That's not why. These values are readonly — if we use SvelteURL then any miscreant will be able to do things like page.url.pathname = '/potato'. It needs to be controlled by the framework

@Gonzalo9823
Copy link
Author

@Rich-Harris Hey! Just to clarify, I don't expect variables using $derived to react whenever I change something in page.url.searchParam. I understand that the page state is readonly. However, I do expect my $derived values to update when navigating to a different page.

// This should not work
const currentPage = $derived(page.url.searchParams.get('page') || '');
page.url.searchParams.set('page', newPage);
// This should work
const currentPage = $derived(page.url.searchParams.get('page') || '');
page.url.searchParams.set('page', newPage);
goto(page.url, { replaceState: true, noScroll: true, keepFocus: true, invalidateAll: true })

Also, while currently goto(page.url) doesn't work, using a link does.

<!-- This works -->
<a data-sveltekit-replacestate data-sveltekit-noscroll data-sveltekit-keepfocus href="/my-page?page=1">1</a>

@Leonidaz
Copy link

That's not why. These values are readonly — if we use SvelteURL then any miscreant will be able to do things like page.url.pathname = '/potato'. It needs to be controlled by the framework

Not sure I understand because at least client-side, while page.url itself is read-only, the pathname is writable via URL.

@eltigerchino
Copy link
Member

eltigerchino commented Dec 19, 2024

That's not why. These values are readonly — if we use SvelteURL then any miscreant will be able to do things like page.url.pathname = '/potato'. It needs to be controlled by the framework

Not sure I understand because at least client-side, while page.url itself is read-only, the pathname is writable via URL.

The intention is that these values are read-only. https://svelte.dev/docs/kit/$app-state

SvelteKit makes three readonly state objects available via the $app/state module — page, navigating and updated.

For example, updating url.pathname doesn't actually navigate to that URL. It's a one way source of truth. You'd have to use goto to actually update the URL.

@Rich-Harris
Copy link
Member

@Gonzalo9823 agree, it's a totally reasonable expectation. We can fix it just by ensuring that page.url is a new object following goto (I think that's the only place where a mutation would go undetected) — #13196

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants