Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
let layoutLoadCount = 0;

// Doesn't read `url`, so SvelteKit should only run it once across child navigations.
export const load = async () => {
layoutLoadCount++;
return { layoutLoadCount };
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script lang="ts">
export let data;
</script>

<p id="layout-load-count">Layout loads: {data.layoutLoadCount}</p>
<slot></slot>
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<h2>Page A</h2>
<a href="/load-dependencies/b">Go to B</a>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<h2>Page B</h2>
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { expect, test } from '@playwright/test';
import { getSpanOp, waitForStreamedSpan } from '@sentry-internal/test-utils';
import { waitForInitialPageload } from './utils';

test("doesn't rerun a wrapped layout server load that doesn't depend on the url", async ({ page }) => {
await waitForInitialPageload(page, { route: '/load-dependencies/a' });

const layoutLoadCount = page.locator('#layout-load-count');
const initialText = await layoutLoadCount.textContent();

const navigationSpanPromise = waitForStreamedSpan('sveltekit-2', span => {
return span.name === '/load-dependencies/b' && getSpanOp(span) === 'navigation' && span.is_segment;
});

await page.getByText('Go to B').click();
await expect(page.getByText('Page B')).toBeVisible();
await navigationSpanPromise;

await expect(layoutLoadCount).toHaveText(initialText!);
});
7 changes: 4 additions & 3 deletions packages/sveltekit/src/client/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { FUNCTION } from '@sentry/conventions/op';
import { captureException } from '@sentry/svelte';
import type { LoadEvent } from '@sveltejs/kit';
import type { SentryWrappedFlag } from '../common/utils';
import { getRouteId, isHttpError, isRedirect } from '../common/utils';
import { getPathname, getRouteId, isHttpError, isRedirect } from '../common/utils';

type PatchedLoadEvent = LoadEvent & Partial<SentryWrappedFlag>;

Expand Down Expand Up @@ -83,7 +83,8 @@ export function wrapLoadWithSentry<T extends (...args: any) => any>(origLoad: T)
addNonEnumerableProperty(patchedEvent, '__sentry_wrapped__', true);

const routeId = getRouteId(event);
const routeOrPathname = routeId ? routeId : event.url.pathname;
const pathname = getPathname(event);
const routeOrPathname = routeId ? routeId : pathname;

const client = getClient();
const hasSpanStreaming = !!client && hasSpanStreamingEnabled(client);
Expand All @@ -97,7 +98,7 @@ export function wrapLoadWithSentry<T extends (...args: any) => any>(origLoad: T)
[CODE_FUNCTION_NAME]: 'load',
[SENTRY_ORIGIN]: 'auto.function.sveltekit',
[SENTRY_SEGMENT_NAME_SOURCE]: routeId ? 'route' : 'url',
[URL_PATH]: event.url.pathname,
[URL_PATH]: pathname,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make sure that dataCollection.urlQueryParams is respected here as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine as the path never contains query strings

...(routeId && { [URL_TEMPLATE]: routeId }),
// Relay infers the description from `code.function.name`, which would drop the route.
...(hasSpanStreaming && { [SENTRY_DESCRIPTION]: routeOrPathname }),
Expand Down
13 changes: 13 additions & 0 deletions packages/sveltekit/src/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ interface EventLike {
route?: {
id?: string | null;
};
url: URL;
untrack<T>(fn: () => T): T;
}

Expand Down Expand Up @@ -43,6 +44,18 @@ export function getRouteId(event: EventLike): string | void {
return;
}

/**
* Get url.pathname from a load event without making the load depend on the URL.
* Without `untrack`, calls the native getter to skip SvelteKit's tracking getter on `event.url`.
*/
export function getPathname(event: EventLike): string {
if (typeof event.untrack === 'function') {
return event.untrack(() => event.url.pathname);
}

return Reflect.get(URL.prototype, 'pathname', event.url);
}

/**
* Determines if a thrown "error" is a Redirect object which SvelteKit users can throw to redirect to another route
* see: https://kit.svelte.dev/docs/modules#sveltejs-kit-redirect
Expand Down
12 changes: 7 additions & 5 deletions packages/sveltekit/src/server-common/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import { FUNCTION } from '@sentry/conventions/op';
import type { LoadEvent, ServerLoadEvent } from '@sveltejs/kit';
import type { SentryWrappedFlag } from '../common/utils';
import { getRouteId } from '../common/utils';
import { getPathname, getRouteId } from '../common/utils';
import { sendErrorToSentry } from './utils';

type PatchedLoadEvent = LoadEvent & SentryWrappedFlag;
Expand Down Expand Up @@ -41,7 +41,8 @@ export function wrapLoadWithSentry<T extends (...args: any) => any>(origLoad: T)
addNonEnumerableProperty(event, '__sentry_wrapped__', true);

const routeId = getRouteId(event) ?? undefined;
const routeOrPathname = routeId ? routeId : event.url.pathname;
const pathname = getPathname(event);
const routeOrPathname = routeId ? routeId : pathname;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super-l: Wouldn't this be the same?

Suggested change
const routeOrPathname = routeId ? routeId : pathname;
const routeOrPathname = routeId || pathname;


const client = getClient();
const hasSpanStreaming = !!client && hasSpanStreamingEnabled(client);
Expand All @@ -56,7 +57,7 @@ export function wrapLoadWithSentry<T extends (...args: any) => any>(origLoad: T)
[CODE_FUNCTION_NAME]: 'load',
[SENTRY_ORIGIN]: 'auto.function.sveltekit',
[SENTRY_SEGMENT_NAME_SOURCE]: routeId ? 'route' : 'url',
[URL_PATH]: event.url.pathname,
[URL_PATH]: pathname,
[HTTP_ROUTE]: routeId,
// Relay infers the description from `code.function.name`, which would drop the route.
...(hasSpanStreaming && { [SENTRY_DESCRIPTION]: routeOrPathname }),
Expand Down Expand Up @@ -113,7 +114,8 @@ export function wrapServerLoadWithSentry<T extends (...args: any) => any>(origSe
// server `load` function's data on every route change. We use `getRouteId` which uses
// SvelteKit 2's `untrack` when available, otherwise getOwnPropertyDescriptor for 1.x.
const routeId = getRouteId(event) ?? undefined;
const routeOrPathname = routeId ? routeId : event.url.pathname;
const pathname = getPathname(event);
const routeOrPathname = routeId ? routeId : pathname;

const client = getClient();
const hasSpanStreaming = !!client && hasSpanStreamingEnabled(client);
Expand All @@ -128,7 +130,7 @@ export function wrapServerLoadWithSentry<T extends (...args: any) => any>(origSe
[SENTRY_ORIGIN]: 'auto.function.sveltekit.server',
[SENTRY_SEGMENT_NAME_SOURCE]: routeId ? 'route' : 'url',
[HTTP_REQUEST_METHOD]: event.request.method,
[URL_PATH]: event.url.pathname,
[URL_PATH]: pathname,
[HTTP_ROUTE]: routeId,
// Relay infers the description from `code.function.name`, which would drop the route.
...(hasSpanStreaming && { [SENTRY_DESCRIPTION]: routeOrPathname }),
Expand Down
25 changes: 23 additions & 2 deletions packages/sveltekit/test/client/load.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { SENTRY_SEGMENT_NAME_SOURCE, SENTRY_OP, SENTRY_ORIGIN } from '@sentry/conventions/attributes';
import { SENTRY_SEGMENT_NAME_SOURCE, SENTRY_OP, SENTRY_ORIGIN, URL_PATH } from '@sentry/conventions/attributes';
import type { Client } from '@sentry/core';
import * as SentryCore from '@sentry/core';
import * as SentrySvelte from '@sentry/svelte';
import type { Load } from '@sveltejs/kit';
import { redirect } from '@sveltejs/kit';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { wrapLoadWithSentry } from '../../src/client/load';
import { createTrackedLoadEvent } from '../utils';

const mockCaptureException = vi.spyOn(SentrySvelte, 'captureException').mockImplementation(() => 'xx');

Expand Down Expand Up @@ -164,7 +165,7 @@ describe('wrapLoadWithSentry', () => {
const wrappedLoad = wrapLoadWithSentry(load);
await wrappedLoad(eventWithUntrack);

expect(untrack).toHaveBeenCalledTimes(1);
expect(untrack).toHaveBeenCalledTimes(2);
expect(mockStartSpan).toHaveBeenCalledWith(
expect.objectContaining({
name: routeIdFromUntrack,
Expand All @@ -176,6 +177,26 @@ describe('wrapLoadWithSentry', () => {
);
});

it("doesn't add route or url dependencies to the load function when untrack is present", async () => {
const { event, trackedReads } = createTrackedLoadEvent({
params: { id: '123' },
route: { id: '/users/[id]' },
url: new URL('http://localhost:3000/users/123'),
});

const wrappedLoad = wrapLoadWithSentry(async () => ({}));
await wrappedLoad(event);

expect(trackedReads).toEqual([]);
expect(mockStartSpan).toHaveBeenCalledWith(
expect.objectContaining({
name: '/users/[id]',
attributes: expect.objectContaining({ [URL_PATH]: '/users/123' }),
}),
expect.any(Function),
);
});

describe('with span streaming enabled', () => {
beforeEach(() => {
vi.spyOn(SentryCore, 'getClient').mockImplementation(
Expand Down
31 changes: 30 additions & 1 deletion packages/sveltekit/test/common/utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it, vi } from 'vitest';
import { getRouteId, isHttpError, isRedirect } from '../../src/common/utils';
import { getPathname, getRouteId, isHttpError, isRedirect } from '../../src/common/utils';

describe('isRedirect', () => {
it.each([
Expand Down Expand Up @@ -102,3 +102,32 @@ describe('getRouteId', () => {
expect(getRouteId({ route: null })).toBeUndefined();
});
});

describe('getPathname', () => {
it('reads url.pathname through untrack when available (SvelteKit 2+)', () => {
const untrack = vi.fn(<T>(fn: () => T) => fn());
const event = {
url: new URL('http://localhost:3000/users/123'),
untrack,
};

// @ts-expect-error - only passing a partial load event here
expect(getPathname(event)).toBe('/users/123');
expect(untrack).toHaveBeenCalledTimes(1);
});

it('bypasses tracked url getters without untrack', () => {
const tracked = vi.fn();
const url = new URL('http://localhost:3000/users/123');
Object.defineProperty(url, 'pathname', {
get() {
tracked();
return '/tracked';
},
});

// @ts-expect-error - only passing a partial load event here
expect(getPathname({ url })).toBe('/users/123');
expect(tracked).not.toHaveBeenCalled();
});
});
36 changes: 33 additions & 3 deletions packages/sveltekit/test/server-common/load.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { SENTRY_SEGMENT_NAME_SOURCE, SENTRY_OP, SENTRY_ORIGIN } from '@sentry/conventions/attributes';
import { SENTRY_SEGMENT_NAME_SOURCE, SENTRY_OP, SENTRY_ORIGIN, URL_PATH } from '@sentry/conventions/attributes';
import type { Client, Event } from '@sentry/core';
import * as SentryCore from '@sentry/core';
import { NodeClient, setCurrentClient } from '@sentry/node';
import type { Load, ServerLoad } from '@sveltejs/kit';
import { error, redirect } from '@sveltejs/kit';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { wrapLoadWithSentry, wrapServerLoadWithSentry } from '../../src/server-common/load';
import { getDefaultNodeClientOptions } from '../utils';
import { createTrackedLoadEvent, getDefaultNodeClientOptions } from '../utils';

const mockCaptureException = vi.spyOn(SentryCore, 'captureException').mockImplementation(() => 'xx');

Expand Down Expand Up @@ -320,7 +320,7 @@ describe('wrapServerLoadWithSentry calls `startSpan`', () => {
const wrappedLoad = wrapServerLoadWithSentry(serverLoad);
await wrappedLoad(eventWithUntrack);

expect(untrack).toHaveBeenCalledTimes(1);
expect(untrack).toHaveBeenCalledTimes(2);
expect(mockStartSpan).toHaveBeenCalledWith(
expect.objectContaining({
name: '/users/[id]',
Expand All @@ -333,6 +333,36 @@ describe('wrapServerLoadWithSentry calls `startSpan`', () => {
});
});

describe.each([
['wrapLoadWithSentry', wrapLoadWithSentry],
['wrapServerLoadWithSentry', wrapServerLoadWithSentry],
])('%s with untrack (SvelteKit 2+)', (_, sentryLoadWrapperFn) => {
it("doesn't add route or url dependencies to the load function", async () => {
const { event, trackedReads } = createTrackedLoadEvent(getServerOnlyArgs());

const wrappedLoad = sentryLoadWrapperFn(() => ({}));
await wrappedLoad(event);

expect(trackedReads).toEqual([]);
expect(mockStartSpan).toHaveBeenCalledWith(
expect.objectContaining({
name: '/users/[id]',
attributes: expect.objectContaining({ [URL_PATH]: '/users/123' }),
}),
expect.any(Function),
);
});

it('still tracks url reads made by the load function itself', async () => {
const { event, trackedReads } = createTrackedLoadEvent(getServerOnlyArgs());

const wrappedLoad = sentryLoadWrapperFn(({ url }: { url: URL }) => ({ path: url.pathname }));
await wrappedLoad(event);

expect(trackedReads).toEqual(['url.pathname']);
});
});

describe('with span streaming enabled', () => {
beforeEach(() => {
vi.spyOn(SentryCore, 'getClient').mockImplementation(
Expand Down
39 changes: 39 additions & 0 deletions packages/sveltekit/test/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,42 @@ export function getDefaultNodeClientOptions(options: Partial<ClientOptions> = {}
...options,
};
}

/**
* Mimics SvelteKit's dependency tracking: reads of `url` and `route` members are recorded
* unless they happen inside `untrack`.
*/
export function createTrackedLoadEvent<T extends object>(
event: T & { url: URL; route: { id: string | null } },
): { event: T & { untrack: <R>(fn: () => R) => R }; trackedReads: string[] } {
const trackedReads: string[] = [];
let tracking = true;

const track = <O extends object>(target: O, name: string): O =>
new Proxy(target, {
get(target, prop) {
if (tracking && typeof prop === 'string') {
trackedReads.push(`${name}.${prop}`);
}
const value = Reflect.get(target, prop);
return typeof value === 'function' ? value.bind(target) : value;
},
});

return {
event: {
...event,
url: track(event.url, 'url'),
route: track(event.route, 'route'),
untrack: <R>(fn: () => R): R => {
tracking = false;
try {
return fn();
} finally {
tracking = true;
}
},
},
trackedReads,
};
}
Loading