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 with SvelteKit #13823

Open
3 tasks done
AliTahir-101 opened this issue Sep 27, 2024 · 8 comments
Open
3 tasks done

False positive pushstate warnings with SvelteKit #13823

AliTahir-101 opened this issue Sep 27, 2024 · 8 comments
Labels
Package: sveltekit Issues related to the Sentry SvelteKit SDK

Comments

@AliTahir-101
Copy link

AliTahir-101 commented Sep 27, 2024

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(...)andhistory.replaceState(...)as these will conflict with SvelteKit's router. Use thepushStateandreplaceStateimports from$app/navigation instead. (client.js, line xxx)

Expected Result

No wanning while using svelte app navigation.

Actual Result

Image

[Warning] Avoid using history.pushState(...)andhistory.replaceState(...)as these will conflict with SvelteKit's router. Use thepushStateandreplaceStateimports from$app/navigation instead. (client.js, line xxx)

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 27, 2024
@github-actions github-actions bot added the Package: browser Issues related to the Sentry Browser SDK label Sep 27, 2024
@AliTahir-101
Copy link
Author

Possible fix: // node_modules/@sentry-internal/browser-utils/build/esm/instrument/history.js

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

@Lms24
Copy link
Member

Lms24 commented Sep 27, 2024

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 @sentry/sveltekit package. The suggested fix would increase bundle size for all browser SDKs so ideally we avoid that :)

@AliTahir-101
Copy link
Author

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 @sentry/sveltekit package. The suggested fix would increase bundle size for all browser SDKs so ideally we avoid that :)

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!

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 27, 2024
@Lms24
Copy link
Member

Lms24 commented Sep 27, 2024

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

@Lms24 Lms24 added Package: sveltekit Issues related to the Sentry SvelteKit SDK Waiting for: Product Owner and removed Package: browser Issues related to the Sentry Browser SDK labels Sep 27, 2024
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 27, 2024
@AliTahir-101
Copy link
Author

AliTahir-101 commented Sep 27, 2024

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.

@Lms24
Copy link
Member

Lms24 commented Sep 27, 2024

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.

@AliTahir-101
Copy link
Author

Thank you for taking the time to look into this. I appreciate your efforts and look forward to any updates you may have!

@getsantry getsantry bot moved this to Waiting for: Community in GitHub Issues with 👀 3 Nov 1, 2024
@getsantry
Copy link

getsantry bot commented Nov 23, 2024

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 Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: sveltekit Issues related to the Sentry SvelteKit SDK
Projects
Status: No status
Development

No branches or pull requests

3 participants