Skip to content

Commit

Permalink
fix(sveltekit): Avoid invalidating data on route changes in `wrapServ…
Browse files Browse the repository at this point in the history
…erLoadWithSentry` (#8801)

The SvelteKit SDK caused server load data to be
invalidated, resulting in said functions being called on every route
change. I initially thought this was related to our Kit-specific client
fetch instrumentation which turned out not to be causing this. Instead,
the culprit is our `wrapServerLoadWithSentry` wrapper:

In the wrapper, we access `event.route.id` to determine the route of the
load function for the span description. Internally, SvelteKit puts a
proxy on certain `event` properties, such as `event.route`. In case any
property of `event.route` was accessed, [SvelteKit marks
this](https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/server/page/load_data.js#L111-L124)
internally and send along a flag to the client. On a route change, the
client would [check this
flag](https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/client/client.js#L572)
and mark the route as invalidated, thereby causing a [call to the load
function](https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/client/client.js#L641)
on each navigation.

---------

Co-authored-by: Kamil Ogórek <kamil.ogorek@gmail.com>
  • Loading branch information
Lms24 and kamilogorek authored Aug 16, 2023
1 parent dc653d0 commit 65df869
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 1 deletion.
6 changes: 5 additions & 1 deletion packages/sveltekit/src/server/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,11 @@ export function wrapServerLoadWithSentry<T extends (...args: any) => any>(origSe

addNonEnumerableProperty(event as unknown as Record<string, unknown>, '__sentry_wrapped__', true);

const routeId = event.route && event.route.id;
// Accessing any member of `event.route` causes SvelteKit to invalidate the
// server `load` function's data on every route change.
// To work around this, we use `Object.getOwnPropertyDescriptor` which doesn't invoke the proxy.
// https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/server/page/load_data.js#L111C3-L124
const routeId = event.route && (Object.getOwnPropertyDescriptor(event.route, 'id')?.value as string | undefined);

const { dynamicSamplingContext, traceparentData, propagationContext } = getTracePropagationData(event);
getCurrentHub().getScope().setPropagationContext(propagationContext);
Expand Down
29 changes: 29 additions & 0 deletions packages/sveltekit/test/server/load.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,4 +400,33 @@ describe('wrapServerLoadWithSentry calls trace', () => {

expect(mockTrace).toHaveBeenCalledTimes(1);
});

it("doesn't invoke the proxy set on `event.route`", async () => {
const event = getServerOnlyArgs();

// simulates SvelteKit adding a proxy to `event.route`
// https://github.com/sveltejs/kit/blob/e133aba479fa9ba0e7f9e71512f5f937f0247e2c/packages/kit/src/runtime/server/page/load_data.js#L111C3-L124
const proxyFn = vi.fn((target: { id: string }, key: string | symbol): any => {
return target[key];
});

event.route = new Proxy(event.route, {
get: proxyFn,
});

const wrappedLoad = wrapServerLoadWithSentry(serverLoad);
await wrappedLoad(event);

expect(mockTrace).toHaveBeenCalledTimes(1);
expect(mockTrace).toHaveBeenCalledWith(
expect.objectContaining({
op: 'function.sveltekit.server.load',
name: '/users/[id]', // <-- this shows that the route was still accessed
}),
expect.any(Function),
expect.any(Function),
);

expect(proxyFn).not.toHaveBeenCalled();
});
});

0 comments on commit 65df869

Please sign in to comment.