-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 with SvelteKit #13823
Comments
Possible fix: import { addHandler, maybeInstrument, supportsHistory, triggerHandlers, fill } from '@sentry/utils';
import { WINDOW } from '../types.js';
let lastHref;
// Check if SvelteKit is in the environment
const isSvelteKit = (() => {
try {
return typeof window !== 'undefined' && window.__sveltekit_dev !== undefined;
} catch {
return false;
}
})();
/**
* Add an instrumentation handler for when a fetch request happens.
* The handler function is called once when the request starts and once when it ends,
* which can be identified by checking if it has an `endTimestamp`.
*
* Use at your own risk, this might break without changelog notice, only used internally.
* @hidden
*/
function addHistoryInstrumentationHandler(handler) {
const type = 'history';
addHandler(type, handler);
maybeInstrument(type, instrumentHistory);
}
function instrumentHistory() {
if (!supportsHistory()) {
return;
}
const oldOnPopState = WINDOW.onpopstate;
WINDOW.onpopstate = function ( ...args) {
const to = WINDOW.location.href;
// keep track of the current URL state, as we always receive only the updated state
const from = lastHref;
lastHref = to;
const handlerData = { from, to };
triggerHandlers('history', handlerData);
if (oldOnPopState) {
// Apparently this can throw in Firefox when incorrectly implemented plugin is installed.
// https://github.com/getsentry/sentry-javascript/issues/3344
// https://github.com/bugsnag/bugsnag-js/issues/469
try {
return oldOnPopState.apply(this, args);
} catch (_oO) {
// no-empty
}
}
};
function historyReplacementFunction(originalHistoryFunction) {
return function ( ...args) {
const url = args.length > 2 ? args[2] : undefined;
if (url) {
// coerce to string (this is what pushState does)
const from = lastHref;
const to = String(url);
// keep track of the current URL state, as we always receive only the updated state
lastHref = to;
const handlerData = { from, to };
triggerHandlers('history', handlerData);
}
// Use SvelteKit navigation if in SvelteKit environment
if (isSvelteKit) {
try {
if (originalHistoryFunction.name === 'pushState') {
import('$app/navigation')
.then(({ pushState }) => pushState(url))
.catch(err => console.error('Failed to push state:', err));
return; // Ensure to exit after calling pushState
} else if (originalHistoryFunction.name === 'replaceState') {
import('$app/navigation')
.then(({ replaceState }) => replaceState(url))
.catch(err => console.error('Failed to replace state:', err));
return; // Ensure to exit after calling replaceState
}
} catch (err) {
console.error(`Failed to import SvelteKit navigation for ${originalHistoryFunction.name}:`, err);
}
}
// Fallback to native behavior
return originalHistoryFunction.apply(this, args);
};
}
fill(WINDOW.history, 'pushState', historyReplacementFunction);
fill(WINDOW.history, 'replaceState', historyReplacementFunction);
}
export { addHistoryInstrumentationHandler };
//# sourceMappingURL=history.js.map |
Hi @AliTahir-101 thanks for writing in! I noticed this in a side project of mine also recently and wanted to follow up on it. I'll look into SvelteKit code to understand where this warning is triggered. Ideally, we find a way to work around this within the |
Hey! My suggestion was meant to provide a quick workaround based on my observations, but I recognize that a more optimal solution would involve adjustments within the sentry/sveltekit package itself. I’m currently focused on my project, so I haven’t had the time to dive deep into debugging the Sentry integration. I appreciate your efforts in finding a more suitable resolution, and I look forward to any updates you may have. Thank you! |
Can you confirm for me that this is only a problem in dev mode? From reading the sveltekit code around it, I think it should be limited to dev but I'd like to make sure that's the case |
Yes brother it seems like it only shows the warning when NODE_ENV is not prod. |
Hey, so I found out why this is happening. The good news: This shouldn't impact any functionality. The bad news: I can't fix this in the SDK without degrading functionality. I think we need to make the check in sveltekit a bit more robust. Let me try if I get somewhere with this. I'll post my findings in the SvelteKit issue you linked. |
Thank you for taking the time to look into this. I appreciate your efforts and look forward to any updates you may have! |
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Is there an existing issue for this?
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/sveltekit
SDK Version
8.32.0
Framework Version
sveltekit 2.5.28
Link to Sentry event
No response
Reproduction Example/SDK Setup
sveltejs/kit#12177
Steps to Reproduce
Just start a sveltekit app with sentry and on first page load you will see the warning.
seems like initiating here
// node_modules/@sentry-internal/browser-utils/build/esm/instrument/history.js
[Warning] Avoid using
history.pushState(...)and
history.replaceState(...)as these will conflict with SvelteKit's router. Use the
pushStateand
replaceStateimports from
$app/navigationinstead. (client.js, line xxx)
Expected Result
No wanning while using svelte app navigation.
Actual Result
[Warning] Avoid using
history.pushState(...)and
history.replaceState(...)as these will conflict with SvelteKit's router. Use the
pushStateand
replaceStateimports from
$app/navigationinstead. (client.js, line xxx)
The text was updated successfully, but these errors were encountered: