diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/+layout.server.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/+layout.server.ts new file mode 100644 index 000000000000..dc09d4b51030 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/+layout.server.ts @@ -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 }; +}; diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/+layout.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/+layout.svelte new file mode 100644 index 000000000000..44e7f425d54b --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/+layout.svelte @@ -0,0 +1,6 @@ + + +

Layout loads: {data.layoutLoadCount}

+ diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/a/+page.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/a/+page.svelte new file mode 100644 index 000000000000..6039c292a519 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/a/+page.svelte @@ -0,0 +1,2 @@ +

Page A

+Go to B diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/b/+page.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/b/+page.svelte new file mode 100644 index 000000000000..f298fcc8d37b --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/load-dependencies/b/+page.svelte @@ -0,0 +1 @@ +

Page B

diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/load-dependencies.test.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/load-dependencies.test.ts new file mode 100644 index 000000000000..1d1cf49de0f0 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/load-dependencies.test.ts @@ -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!); +}); diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index a1a78995ce0e..a96634963899 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -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; @@ -83,7 +83,8 @@ export function wrapLoadWithSentry 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); @@ -97,7 +98,7 @@ export function wrapLoadWithSentry 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, ...(routeId && { [URL_TEMPLATE]: routeId }), // Relay infers the description from `code.function.name`, which would drop the route. ...(hasSpanStreaming && { [SENTRY_DESCRIPTION]: routeOrPathname }), diff --git a/packages/sveltekit/src/common/utils.ts b/packages/sveltekit/src/common/utils.ts index b5621cddd059..73326f81e146 100644 --- a/packages/sveltekit/src/common/utils.ts +++ b/packages/sveltekit/src/common/utils.ts @@ -15,6 +15,7 @@ interface EventLike { route?: { id?: string | null; }; + url: URL; untrack(fn: () => T): T; } @@ -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 diff --git a/packages/sveltekit/src/server-common/load.ts b/packages/sveltekit/src/server-common/load.ts index 5a6dfd93100b..cfad7bcf9621 100644 --- a/packages/sveltekit/src/server-common/load.ts +++ b/packages/sveltekit/src/server-common/load.ts @@ -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; @@ -41,7 +41,8 @@ export function wrapLoadWithSentry 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; const client = getClient(); const hasSpanStreaming = !!client && hasSpanStreamingEnabled(client); @@ -56,7 +57,7 @@ export function wrapLoadWithSentry 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 }), @@ -113,7 +114,8 @@ export function wrapServerLoadWithSentry 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); @@ -128,7 +130,7 @@ export function wrapServerLoadWithSentry 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 }), diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index 5978a2bffc01..c975b7e9fcd9 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -1,4 +1,4 @@ -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'; @@ -6,6 +6,7 @@ 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'); @@ -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, @@ -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( diff --git a/packages/sveltekit/test/common/utils.test.ts b/packages/sveltekit/test/common/utils.test.ts index 6650a491c80f..015f6b8ac573 100644 --- a/packages/sveltekit/test/common/utils.test.ts +++ b/packages/sveltekit/test/common/utils.test.ts @@ -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([ @@ -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((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(); + }); +}); diff --git a/packages/sveltekit/test/server-common/load.test.ts b/packages/sveltekit/test/server-common/load.test.ts index 97b3a6390c08..9758c4852903 100644 --- a/packages/sveltekit/test/server-common/load.test.ts +++ b/packages/sveltekit/test/server-common/load.test.ts @@ -1,4 +1,4 @@ -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'; @@ -6,7 +6,7 @@ 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'); @@ -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]', @@ -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( diff --git a/packages/sveltekit/test/utils.ts b/packages/sveltekit/test/utils.ts index 29d32e8df2c7..3f8e0deca433 100644 --- a/packages/sveltekit/test/utils.ts +++ b/packages/sveltekit/test/utils.ts @@ -11,3 +11,42 @@ export function getDefaultNodeClientOptions(options: Partial = {} ...options, }; } + +/** + * Mimics SvelteKit's dependency tracking: reads of `url` and `route` members are recorded + * unless they happen inside `untrack`. + */ +export function createTrackedLoadEvent( + event: T & { url: URL; route: { id: string | null } }, +): { event: T & { untrack: (fn: () => R) => R }; trackedReads: string[] } { + const trackedReads: string[] = []; + let tracking = true; + + const track = (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: (fn: () => R): R => { + tracking = false; + try { + return fn(); + } finally { + tracking = true; + } + }, + }, + trackedReads, + }; +}