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

Using the SvelteKit SDK breaks SSR functionality of TanStack Query #8818

Closed
3 tasks done
kevinrenskers opened this issue Aug 15, 2023 · 6 comments · Fixed by #9071
Closed
3 tasks done

Using the SvelteKit SDK breaks SSR functionality of TanStack Query #8818

kevinrenskers opened this issue Aug 15, 2023 · 6 comments · Fixed by #9071

Comments

@kevinrenskers
Copy link

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

7.64.0

Framework Version

7.64.0

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

When using your SvelteKit SDK, and specifically your Vite plugin for SvelteKit, the preloading / SSR functionality of TanStack Query for Svelte seems broken.

  1. Clone my https://github.com/kevinrenskers/sveltequery-sentry-sveltekit repo and npm install
  2. npm run dev, open http://127.0.0.1:5173
  3. Open the javascript console and then click on the "Articles" link in the top

You will see that the posts array is first undefined, and then it gets data:

Screenshot 2023-08-15 at 14 29 45

Now edit vite.config.ts and remove sentrySvelteKit() from the plugins. Restart the dev server and do the same thing again: start on the homepage and then go to articles. Now the posts array is immediately defined:

Screenshot 2023-08-15 at 14 28 47

It does have a real effect in my website, where articles pop in a fraction of a second later instead of already being there. Basically SSR seems broken.

Expected Result

I don't know if the problem lies with your sentrySvelteKit Vite plugin which modifies the fetch function, or if the problem lies with Query which can't deal with these changes. But I would expect that installing your SvelteKit SDK and Vite plugin to not change the behavior of my site when using Query.

Also reported in their repo: TanStack/query#5871

Actual Result

See repro steps above

@AbhiPrasad
Copy link
Member

This should be fixed by #8802 - I’ll let @Lms24 confirm though

@Lms24
Copy link
Member

Lms24 commented Aug 17, 2023

Hi @kevinrenskers thanks for reporting and for the great reproduction! I cloned it and tried it out. Unfortunately, #8802 doesn't fix your issues because it turns out, it's not related to fetch. It's very similar to #8610 just that this time, it's not about the server-side load functions but about the client-side (+page.ts) universal load function.

The problem is that in our wrapLoadWithSentry wrapper, we access event.route.id. SvelteKit internally listens for accesses to route.id and later on checks if there was an access. With our wrapLoadWithSentry wrapper in place, there'll always be such an access, regardless of if you use event.route.id in your load function or not. As a result, your prefetch (I believe) is invalidated and you have to load again.

While we were able to work around this on the server-side (#8801), the way how Kit attaches its listener on the client side doesn't permit us to do the same thing again. In fact, I can't think of a way to still get the route id for our span description without triggering this invalidation logic.

I would like to get this fixed in SvelteKit itself but in our experience, this takes quite a while, so our other option is to remove the route id access from our wrappers. Not ideal but not a total deal breaker either.

In the meantime, you can work around this by disabeling the client-side auto-instrumentation in sentrySvelteKit:

import { sveltekit } from "@sveltejs/kit/vite";
import { sentrySvelteKit } from "@sentry/sveltekit";

export default {
  plugins: [
    sentrySvelteKit({
      autoInstrument: {
        load: false,
        serverLoad: true,
      },
      // OR disable auto-instrumentation entirely:
      // autoInstrument: false,
    }),
    sveltekit(),
  ],
  // ... rest of your Vite config
};

Sorry that there isn't a perfect fix. I'll keep you posted.

@Lms24
Copy link
Member

Lms24 commented Sep 4, 2023

Good news: My PR to SvelteKit was merged so we're ready now to implement the workaround on the client side. I'll try to get to it this week. Concretely, we need to do two things:

@Lms24
Copy link
Member

Lms24 commented Sep 20, 2023

Hi @kevinrenskers thanks for your patience! I opened #9071 to fix this based on the SvelteKit change. So in order to fully leverage this fix, please also update your SvelteKit version to >= 1.24.0. We'll ship this fix in the next SDK release which probably happens this week or at the beginning of next week.

Lms24 added a commit that referenced this issue Oct 2, 2023
… functions (#9071)

This patch fixes a data invalidation issue that appeared on the client side
when auto-instrumenting or manually wrapping universal `load` functions.
Because our `wrapLoadWithSentry` function accessed `event.route.id`, the
data returned from `load` would be invalidated on a route change. As
reported in #8818 , this caused prefetched, actually still valid data to be
invalidated during the navigation to the page, causing another `load`
invocation.

To avoid this invalidation, we change the way how we read the route.id,
to first use `Object.getOwnPropertyDescriptor` which doesn't trigger the
proxy that listens to accesses. This, however, will only work for
`@sveltejs/kit>=1.24.0` which includes a
[change](sveltejs/kit#10576) to the Kit-internal
proxy. For older versions of kit, we continue to directly read from
`event.route.id`, which will still cause invalidations. Not ideal but
IMO the best we can do without loosing data.

closes #8818
@Lms24
Copy link
Member

Lms24 commented Oct 2, 2023

Unfortunately, it took a little longer than expected but I just merged #9071. We'll release a new SDK version today or tomorrow by the latest.

@AbhiPrasad
Copy link
Member

Released as part of https://github.com/getsentry/sentry-javascript/releases/tag/7.73.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
3 participants