Skip to content

Commit

Permalink
fix(sveltekit): Check for cached requests in client-side fetch instru…
Browse files Browse the repository at this point in the history
…mentation (#8391)

As outlined in
#8174 (comment),
our current SvelteKit fetch instrumentation breaks SvelteKit's request
caching mechanism. This is problematic as `fetch` requests from
universal `load` functions were made again on the client side during
hydration although the response was already cached from the initial
server-side request.

The reason for the cache miss is that in the instrumentation we add our
tracing headers to the requests, which lead to a different cache key
than the one produced on the server side.

This fix vendors in code from [the SvelteKit
repo](https://github.com/sveltejs/kit) so that we can perform the same
cache lookup in our instrumentation. If the lookup was successful (-->
cache hit), we won't attach any headers or create breadcrumbs to 1. let
Kit's fetch return the cached response and 2. not add spans for a fetch
request that didn't even happen.
  • Loading branch information
Lms24 committed Jun 26, 2023
1 parent 98d3916 commit e583fe9
Show file tree
Hide file tree
Showing 7 changed files with 250 additions and 1 deletion.
7 changes: 7 additions & 0 deletions packages/sveltekit/src/client/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type { LoadEvent } from '@sveltejs/kit';

import type { SentryWrappedFlag } from '../common/utils';
import { isRedirect } from '../common/utils';
import { isRequestCached } from './vendor/lookUpCache';

type PatchedLoadEvent = LoadEvent & Partial<SentryWrappedFlag>;

Expand Down Expand Up @@ -153,6 +154,11 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch
return new Proxy(originalFetch, {
apply: (wrappingTarget, thisArg, args: Parameters<LoadEvent['fetch']>) => {
const [input, init] = args;

if (isRequestCached(input, init)) {
return wrappingTarget.apply(thisArg, args);
}

const { url: rawUrl, method } = parseFetchArgs(args);

// TODO: extract this to a util function (and use it in breadcrumbs integration as well)
Expand Down Expand Up @@ -196,6 +202,7 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch

patchedInit.headers = headers;
}

let fetchPromise: Promise<Response>;

const patchedFetchArgs = [input, patchedInit];
Expand Down
57 changes: 57 additions & 0 deletions packages/sveltekit/src/client/vendor/buildSelector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/* eslint-disable @sentry-internal/sdk/no-optional-chaining */

// Vendored from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js
// with types only changes.

// The MIT License (MIT)

// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors)

// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files(the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:

// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.

// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

import { hash } from './hash';

/**
* Build the cache key for a given request
* @param {URL | RequestInfo} resource
* @param {RequestInit} [opts]
*/
export function build_selector(resource: URL | RequestInfo, opts: RequestInit | undefined): string {
const url = JSON.stringify(resource instanceof Request ? resource.url : resource);

let selector = `script[data-sveltekit-fetched][data-url=${url}]`;

if (opts?.headers || opts?.body) {
/** @type {import('types').StrictBody[]} */
const values = [];

if (opts.headers) {
// @ts-ignore - TS complains but this is a 1:1 copy of the original code and apparently it works
values.push([...new Headers(opts.headers)].join(','));
}

if (opts.body && (typeof opts.body === 'string' || ArrayBuffer.isView(opts.body))) {
values.push(opts.body);
}

selector += `[data-hash="${hash(...values)}"]`;
}

return selector;
}
51 changes: 51 additions & 0 deletions packages/sveltekit/src/client/vendor/hash.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/* eslint-disable no-bitwise */

// Vendored from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/hash.js
// with types only changes.

// The MIT License (MIT)

// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors)

// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files(the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:

// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.

// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

import type { StrictBody } from '@sveltejs/kit/types/internal';

/**
* Hash using djb2
* @param {import('types').StrictBody[]} values
*/
export function hash(...values: StrictBody[]): string {
let hash = 5381;

for (const value of values) {
if (typeof value === 'string') {
let i = value.length;
while (i) hash = (hash * 33) ^ value.charCodeAt(--i);
} else if (ArrayBuffer.isView(value)) {
const buffer = new Uint8Array(value.buffer, value.byteOffset, value.byteLength);
let i = buffer.length;
while (i) hash = (hash * 33) ^ buffer[--i];
} else {
throw new TypeError('value must be a string or TypedArray');
}
}

return (hash >>> 0).toString(36);
}
79 changes: 79 additions & 0 deletions packages/sveltekit/src/client/vendor/lookUpCache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/* eslint-disable no-bitwise */

// Parts of this code are taken from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js
// Attribution given directly in the function code below

// The MIT License (MIT)

// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors)

// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files(the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:

// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.

// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

import { WINDOW } from '@sentry/svelte';
import { getDomElement } from '@sentry/utils';

import { build_selector } from './buildSelector';

/**
* Checks if a request is cached by looking for a script tag with the same selector as the constructed selector of the request.
*
* This function is a combination of the cache lookups in sveltekit's internal client-side fetch functions
* - initial_fetch (used during hydration) https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L76
* - subsequent_fetch (used afterwards) https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L98
*
* Parts of this function's logic is taken from SvelteKit source code.
* These lines are annotated with attribution in comments above them.
*
* @param input first fetch param
* @param init second fetch param
* @returns true if a cache hit was encountered, false otherwise
*/
export function isRequestCached(input: URL | RequestInfo, init: RequestInit | undefined): boolean {
// build_selector call copied from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L77
const selector = build_selector(input, init);

const script = getDomElement<HTMLScriptElement>(selector);

if (!script) {
return false;
}

// If the script has a data-ttl attribute, we check if we're still in the TTL window:
try {
// ttl retrieval taken from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L83-L84
const ttl = Number(script.getAttribute('data-ttl')) * 1000;

if (isNaN(ttl)) {
return false;
}

if (ttl) {
// cache hit determination taken from: https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L105-L106
return (
WINDOW.performance.now() < ttl &&
['default', 'force-cache', 'only-if-cached', undefined].includes(init && init.cache)
);
}
} catch {
return false;
}

// Otherwise, we check if the script has a content and return true in that case
return !!script.textContent;
}
7 changes: 6 additions & 1 deletion packages/sveltekit/test/client/load.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ vi.mock('@sentry/svelte', async () => {
};
});

vi.mock('../../src/client/vendor/lookUpCache', () => {
return {
isRequestCached: () => false,
};
});

const mockTrace = vi.fn();

const mockedBrowserTracing = {
Expand Down Expand Up @@ -433,7 +439,6 @@ describe('wrapLoadWithSentry', () => {
['is undefined', undefined],
["doesn't have a `getClientById` method", {}],
])("doesn't instrument fetch if the client %s", async (_, client) => {
// @ts-expect-error: we're mocking the client
mockedGetClient.mockImplementationOnce(() => client);

async function load(_event: Parameters<Load>[0]): Promise<ReturnType<Load>> {
Expand Down
45 changes: 45 additions & 0 deletions packages/sveltekit/test/client/vendor/lookUpCache.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { JSDOM } from 'jsdom';
import { vi } from 'vitest';

import { isRequestCached } from '../../../src/client/vendor/lookUpCache';

globalThis.document = new JSDOM().window.document;

vi.useFakeTimers().setSystemTime(new Date('2023-06-22'));
vi.spyOn(performance, 'now').mockReturnValue(1000);

describe('isRequestCached', () => {
it('should return true if a script tag with the same selector as the constructed request selector is found', () => {
globalThis.document.body.innerHTML =
'<script type="application/json" data-sveltekit-fetched data-url="/api/todos/1">{"status":200}</script>';

expect(isRequestCached('/api/todos/1', undefined)).toBe(true);
});

it('should return false if a script with the same selector as the constructed request selector is not found', () => {
globalThis.document.body.innerHTML = '';

expect(isRequestCached('/api/todos/1', undefined)).toBe(false);
});

it('should return true if a script with the same selector as the constructed request selector is found and its TTL is valid', () => {
globalThis.document.body.innerHTML =
'<script type="application/json" data-sveltekit-fetched data-url="/api/todos/1" data-ttl="10">{"status":200}</script>';

expect(isRequestCached('/api/todos/1', undefined)).toBe(true);
});

it('should return false if a script with the same selector as the constructed request selector is found and its TTL is expired', () => {
globalThis.document.body.innerHTML =
'<script type="application/json" data-sveltekit-fetched data-url="/api/todos/1" data-ttl="1">{"status":200}</script>';

expect(isRequestCached('/api/todos/1', undefined)).toBe(false);
});

it("should return false if the TTL is set but can't be parsed as a number", () => {
globalThis.document.body.innerHTML =
'<script type="application/json" data-sveltekit-fetched data-url="/api/todos/1" data-ttl="notANumber">{"status":200}</script>';

expect(isRequestCached('/api/todos/1', undefined)).toBe(false);
});
});
5 changes: 5 additions & 0 deletions packages/sveltekit/test/vitest.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,8 @@ export function setup() {
};
});
}

if (!globalThis.fetch) {
// @ts-ignore - Needed for vitest to work with SvelteKit fetch instrumentation
globalThis.Request = class Request {};
}

0 comments on commit e583fe9

Please sign in to comment.