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

False positive pushstate warnings #11671

Closed
dummdidumm opened this issue Jan 18, 2024 · 9 comments · Fixed by #11858
Closed

False positive pushstate warnings #11671

dummdidumm opened this issue Jan 18, 2024 · 9 comments · Fixed by #11858
Labels
bug Something isn't working

Comments

@dummdidumm
Copy link
Member

          This PR, released as part of `v2.3.4` results in a new warning that wasn't previously there:
[Warning] 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. (client.js, line 2580)

I'm not using history as suggested by the warning. I'm using goto in a couple of places, from $app/navigation. I haven't changed anything. The warning is not present with v2.3.3.

From a quick test, commenting out all the goto calls still resulted in the warning.

Originally posted by @lukasmalkmus in #11657 (comment)

I think this happens because when a library like Google Analytics monkey patches pushstate it will be next in the call stack, resulting in the false positive. I'm not sure how to solve this other than checking the whole stack for an occurrence of the client file, which may result in false negatives.

@mgrubinger
Copy link
Contributor

I get this warning as soon as I do use history.pushState().

As much as I appreciate the work that went into pushState from '$app/navigation' I'm not sure I can actually use it in our (very special, indeed) setup. But SvelteKit overriding/extending a browser API (history.pushState) makes me feel uneasy.

I'd much prefer to be able to use history.pushState as usual, but still have the added ease-of-use of $app/navigation.pushState() if I chose to use it.

@dummdidumm dummdidumm added the bug Something isn't working label Jan 26, 2024
@dummdidumm
Copy link
Member Author

SvelteKit is not overriding a browser API - we only patch the pushState/replaceState at dev time to add this warning in case someone would call it themselves directly. SvelteKit always used the history state to place state on it, so this warning just surfaced potentially dangerous behavior anyway.
That said, I'm wondering if we should not warn in dev mode if the user did preserve the internal SvelteKit state (i.e. the SvelteKit history index etc entries are still there). @Rich-Harris you originally added the warning in #11307 to discourage usage. What dangers do you saw besides not using pushState/replaceState which may not behave the way people want them to (because of #11503 (comment))?

@stephenlrandall
Copy link
Contributor

Getting a false positive as well, also with any goto and replaceState calls commented out. This is with SvelteKit 2.5.0.

@conoremclaughlin
Copy link

Also receiving a false positive when calling goto. Given the warnings we're receiving, it would be helpful if the documentation was also updated to include multiple examples of replaceState / pushState. Page state as a required parameter for replaceState is confusing and inconsistent with the original History API.

We'd ideally do a drop-in replacement for history.replaceState when changing the query parameters via shallow routing, but we cannot do so currently and instead have to wrestle with the unclear example usage of replaceState. StackOverflow currently recommends using goto but as mentioned in #11465, this behavior is also inconsistent. If replaceState and pushState could mirror the History API as closely as possible, thorough documentation of example usage wouldn't be necessary. Thanks so much for the help!

dummdidumm pushed a commit that referenced this issue Feb 19, 2024
fixes #11671

The warning currently incorrectly fires when we use history.pushState() internally such as clicking on a link. You can test this in a stackblitz starter project. This is because one of the early exit conditions in the warning method is failing incorrectly.

import.meta.url returns the client.js file with a query parameter when loading it from node_modules. This causes the substring search for the module filename in the error stack to always return false (because of the query params that are not present in the error stack)

The solution is to simply return the URL without the query parameters if any.
@jycouet
Copy link
Contributor

jycouet commented Feb 29, 2024

I still have the issue, but I'm not sure what I can do since it doesn't look like file I own.

Stack is refering to:

Promise.all([
  import("/@fs/.../node_modules/.pnpm/@sveltejs+kit@2.5.2_@sveltejs+vite-plugin-svelte@3.0.2_svelte@4.2.7_vite@5.1.4/node_modules/@sveltejs/kit/src/runtime/client/entry.js"),
  import("/@fs/.../apps/cadb/.svelte-kit/generated/client/app.js")
  ]).then(([kit, app]) => {
    kit.start(app, element);
  });

And the first goto is here:
Capture d’écran 2024-02-29 à 09 32 04


At first, I was thinking that it was due to paoloricciuti/sveltekit-search-params#66 but it doens't look like.

@dummdidumm
Copy link
Member Author

Can you open a new issue with a reproduction?

@jycouet
Copy link
Contributor

jycouet commented Feb 29, 2024

I tried and didn't manage to reproduce in a small app :/
And in my "big" private project, I looked at all my goto & co, but was not able to identify where is my guilty code.
The only easy part on my side was to do this screenshot about the stacktrace with all kit files + Hydration.

I'll try to add more time into finding the root cause, but can't right now.

@pboling
Copy link

pboling commented Mar 27, 2024

Getting this when using Open Telemetry tracing on client side. The library is using push state, none of my internal code calls push state.

@fromaline
Copy link

Hi!
I get the same warning when using history.replaceState intentionally. I read through the docs and GitHub issues, but I don't quite understand, how to replicate the behavior of history.replaceState with replaceState from SvelteKit.
I need to update query params without changing page state and running the load function, but when I do this with replaceState I get an error:

client.js?v=c80da5fa:1921 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '$set')
    at replaceState (client.js?v=c80da5fa:1921:7)

My code roughly looks like this:

import { replaceState } from "$app/navigation"

export const replaceStateWithQuery = (values: Record<string, string | string[] | undefined>) => {
  const url = new URL(window.location.toString())

  for (const [key, value] of Object.entries(values)) {
    if (value && Array.isArray(value)) {
      value.forEach(val => {
        url.searchParams.append(key, val)
      })
    } else if (value) {
      url.searchParams.set(key, value)
    } else {
      url.searchParams.delete(key)
    }
  }

  replaceState(url, {})
}

Then I run replaceStateWithQuery when a user changes some filters on the page to persist the state of the filters in URL.

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.

7 participants