sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.62k stars 1.92k forks source link

SvelteKit 2: load url.searchParams bug #11503

Open martonx opened 9 months ago

martonx commented 9 months ago

Describe the bug

When we changed searchParams, and force to reload sveltekit data, url.searchParams remain the same value as before the change.

Reproduction

Reproduction:

https://stackblitz.com/edit/sveltejs-kit-template-default-xeika8?file=src%2Froutes%2F%2Bpage.svelte

click on the change url button. This will do a window.history.pushState with new searchParams then invalidate data, so it'll trigger the page.js load function. In load function check url.searchParams (consol.log writes it).

The reproduction is a bit bad as this sandbox environment doesn't visibly change the url, but the reproduction is working locally or from any kind of hosting method.

This worked with SvelteKit 1.30.x. It is broken just after the SvelteKit 2.0 update. You can reproduce it with old and current SvelteKit.

Logs

No response

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (16) x64 AMD Ryzen 7 5800X 8-Core Processor
    Memory: 6.48 GB / 15.93 GB
  Binaries:
    Node: 18.16.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 9.5.1 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.7.5 - ~\AppData\Local\pnpm\pnpm.EXE
  Browsers:
    Edge: Chromium (120.0.2210.91)
    Internet Explorer: 11.0.22621.1

Severity

critical

Additional Information

No response

martonx commented 9 months ago

I'm sorry severity is not annoyance, but this is a critical issue, as this one bug blocks us to upgrade to sveltekit 2.0

CaptainCodeman commented 9 months ago

The console shows the issue and solution:

Avoid using history.pushState(...) and history.replaceState(...) as these will conflict with SvelteKit's router. Use the pushState and replaceState imports from $app/navigation instead.

With that change, it works as expected.

dummdidumm commented 9 months ago

This was changed in #11307 - @Rich-Harris could you elaborate on why the change to use current.url instead of location.href in invalidate was necessary? Also, this uncovers a hole in the API: pushState is tightly coupled to shallow routing - you can't push state in the regular way that also updates $page.url. This together with the change to invalidate means you're now unable to get this working (@CaptainCodeman are you sure this worked fo you? I can't get it to work with changing this to use pushState from $app/navigation).

That said, @martonx why aren't you using goto in this case? Does this way allow you more fine-grained control over which things to reload / your problem is that goto would reload too much?

CaptainCodeman commented 9 months ago

I thought it did last night, but stackblitz isn't working for me this morning to confirm, I may have been mistaken.

martonx commented 9 months ago

@dummdidumm I don't use goto because of I don't want to navigate in real. I just want to change the url, and fetch some fresh data. Actually this is a deep linked filtering in an interactive page. When you are changing the filters, I'm doing the filtering in the load method (this is why I use invalidate), and I'm keeping in sync the url query string params. This use case, this code works perfect with SvelteKit 1.30.x

I tried goto now. The issue with goto it overwrites the current page state for example scroll position. So if I switch these lines:

window.history.pushState(null, '', ${path}?${params.toString()}); invalidate('app:blogs');

to:

goto(${path}?${params.toString()});

The filtering is working, but the page is scrolling to the top, I don't know why. This is not visible in the repro code, but very frustrating in my real code, where the filters, and the filter results are not on the top of the page.

martonx commented 9 months ago

@CaptainCodeman

I changed this line:

window.history.pushState(null, '', ${path}?${params.toString()});

to

pushState(${path}?${params.toString()}, $page.state);

nothing changed, nothing fixed. Am I right with this pushState usage? The documentation has no example of how should we use SvelteKit's app/navigation's own pushState method.

AlexisGado commented 9 months ago

It looks similar to the issue I opened 2 weeks ago (https://github.com/sveltejs/kit/issues/11465).

It seems that pushState and replaceState only work for shallow routing (url stays the same), but it's never made clear in the documentation (and the deprecation warning for history indicates otherwise).

eltigerchino commented 7 months ago

@dummdidumm I don't use goto because of I don't want to navigate in real. I just want to change the url, and fetch some fresh data. Actually this is a deep linked filtering in an interactive page. When you are changing the filters, I'm doing the filtering in the load method (this is why I use invalidate), and I'm keeping in sync the url query string params. This use case, this code works perfect with SvelteKit 1.30.x

I tried goto now. The issue with goto it overwrites the current page state for example scroll position. So if I switch these lines:

window.history.pushState(null, '', ${path}?${params.toString()}); invalidate('app:blogs');

to:

goto(${path}?${params.toString()});

The filtering is working, but the page is scrolling to the top, I don't know why. This is not visible in the repro code, but very frustrating in my real code, where the filters, and the filter results are not on the top of the page.

I think what you want is goto(${path}?${params.toString()}, { replaceState: true, noScroll: true });. replaceState replaces the url without navigating and noScroll prevents scrolling to the top of the page. https://kit.svelte.dev/docs/modules#$app-navigation-goto

Or better yet, use a GET action form that does this for you with progressive enhancement https://kit.svelte.dev/docs/form-actions#get-vs-post

The real issue is that after using pushState, I suspect the sveltekit tracked URL is not updated correctly so the invalidation called in the reproduction does not console log the updated URL search query params.

f-elix commented 3 months ago

I also ran into this problem, and its very annoying. For now I can work around it with this:

window.history.replaceState(history.state, '', url);
eltigerchino commented 2 weeks ago

I'm not sure how to fix this without updating the internal current.url so that it invalidates using the URL that was passed to pushState. However, updating current.url also updates the page store URL, which is what we don't want when using pushState.

Some notes on shallow routing with pushState / replaceState: