-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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: prevent false positive history.pushState
warnings
#11858
Conversation
🦋 Changeset detectedLatest commit: f6d7017 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing all the different cases!
I'm still getting the warning when using Sentry integration. As soon as Not sure whether it should be reported here or in Sentry repo. |
Thanks for reporting this. Can you open a new issue with a minimal reproduction? |
I'm experiencing the same warning when using Sentry integration. The warning appears on the first navigation after a hard page load (but not on subsequent client-side navigation between pages). Here's a minimal repo reproducing the issue: https://github.com/linsen/sveltekit-sentry-warning Should I open a new issue for this? I don't see one that's currently open. |
Hi Linsen. Thanks for reporting this. Yes, you’re encouraged to open a new issue for this. |
I'm not using Sentry nor do I use My svelte/kit versions:
|
Please open a new issue with a minimal reproduction |
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 theclient.js
file with a query parameter when loading it fromnode_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)kit/packages/kit/src/runtime/client/client.js
Line 79 in 53412fd
The solution is to simply return the URL without the query parameters if any.
No test because so far it's only reproducible when Vite is loading SvelteKit from
node_modules
rather than being linked through the workspace (how it's currently setup in the kit repository).Aside
This addresses the original issue of:
but it does not address the possibility thatwarn
checks the wrong stack entry. This can happen when other scripts also monkey patchhistory.pushState
causing ourclient.js
to appear earlier or later in the call stack.I think we're safe from third-party scripts monkey patching and upsetting the call stack order. I've tested monkey patching it in:
and they get called in that order, but
warn
is always first in the call stack, followed by theclient.js
monkey patch.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:
.Edits