From f867447ff7a66275fae8221dadb7ea69ad61db52 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 10 Mar 2023 16:41:15 +0100 Subject: [PATCH 1/9] feat(sveltekit): Add wrapper for server load function --- packages/sveltekit/src/client/handleError.ts | 2 +- packages/sveltekit/src/server/handleError.ts | 2 +- packages/sveltekit/src/server/index.ts | 1 + packages/sveltekit/src/server/load.ts | 40 ++++++++++ .../sveltekit/test/server/handleError.test.ts | 2 +- packages/sveltekit/test/server/load.test.ts | 74 +++++++++++++++++++ 6 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 packages/sveltekit/src/server/load.ts create mode 100644 packages/sveltekit/test/server/load.test.ts diff --git a/packages/sveltekit/src/client/handleError.ts b/packages/sveltekit/src/client/handleError.ts index 5490a3e20980..8cc781c70686 100644 --- a/packages/sveltekit/src/client/handleError.ts +++ b/packages/sveltekit/src/client/handleError.ts @@ -16,7 +16,7 @@ export function handleErrorWithSentry(handleError?: HandleClientError): HandleCl captureException(input.error, scope => { scope.addEventProcessor(event => { addExceptionMechanism(event, { - type: 'sveltekit', + type: 'instrument', handled: false, }); return event; diff --git a/packages/sveltekit/src/server/handleError.ts b/packages/sveltekit/src/server/handleError.ts index 449ca65e19ce..5dce61ec9bc4 100644 --- a/packages/sveltekit/src/server/handleError.ts +++ b/packages/sveltekit/src/server/handleError.ts @@ -16,7 +16,7 @@ export function handleErrorWithSentry(handleError?: HandleServerError): HandleSe captureException(input.error, scope => { scope.addEventProcessor(event => { addExceptionMechanism(event, { - type: 'sveltekit', + type: 'instrument', handled: false, }); return event; diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index 1e3aed034f1e..c7784d870c56 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -2,3 +2,4 @@ export * from '@sentry/node'; export { init } from './sdk'; export { handleErrorWithSentry } from './handleError'; +export { wrapLoadWithSentry } from './load'; diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts new file mode 100644 index 000000000000..c4fb0b71ec9a --- /dev/null +++ b/packages/sveltekit/src/server/load.ts @@ -0,0 +1,40 @@ +import { captureException } from '@sentry/node'; +import { addExceptionMechanism, objectify } from '@sentry/utils'; +// eslint-disable-next-line import/no-unresolved +import type { ServerLoad } from '@sveltejs/kit'; + +/** + * Wrap load function with Sentry + * + * @param origLoad SvelteKit user defined load function + */ +export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { + return new Proxy(origLoad, { + apply: async (wrappingTarget, thisArg, args: Parameters) => { + try { + return await wrappingTarget.apply(thisArg, args); + } catch (e) { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. + const objectifiedErr = objectify(e); + + captureException(objectifiedErr, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'instrument', + handled: false, + data: { + function: 'load', + }, + }); + return event; + }); + + return scope; + }); + + throw objectifiedErr; + } + }, + }); +} diff --git a/packages/sveltekit/test/server/handleError.test.ts b/packages/sveltekit/test/server/handleError.test.ts index da73ca0eef5d..d0a23667dad1 100644 --- a/packages/sveltekit/test/server/handleError.test.ts +++ b/packages/sveltekit/test/server/handleError.test.ts @@ -12,7 +12,7 @@ const mockCaptureException = vi.fn(); let mockScope = new Scope(); vi.mock('@sentry/node', async () => { - const original = (await vi.importActual('@sentry/core')) as any; + const original = (await vi.importActual('@sentry/node')) as any; return { ...original, captureException: (err: unknown, cb: (arg0: unknown) => unknown) => { diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts new file mode 100644 index 000000000000..04ed34d45a5d --- /dev/null +++ b/packages/sveltekit/test/server/load.test.ts @@ -0,0 +1,74 @@ +import { Scope } from '@sentry/node'; +// eslint-disable-next-line import/no-unresolved +import type { ServerLoad } from '@sveltejs/kit'; + +import { wrapLoadWithSentry } from '../../src/server/load'; + +const mockCaptureException = jest.fn(); +let mockScope = new Scope(); + +jest.mock('@sentry/node', () => { + const original = jest.requireActual('@sentry/node'); + return { + ...original, + captureException: (err: unknown, cb: (arg0: unknown) => unknown) => { + cb(mockScope); + mockCaptureException(err, cb); + return original.captureException(err, cb); + }, + }; +}); + +const mockAddExceptionMechanism = jest.fn(); + +jest.mock('@sentry/utils', () => { + const original = jest.requireActual('@sentry/utils'); + return { + ...original, + addExceptionMechanism: (...args: unknown[]) => mockAddExceptionMechanism(...args), + }; +}); + +function getById(_id?: string) { + throw new Error('error'); +} + +async function erroringLoad({ params }: Parameters[0]): Promise> { + return { + post: getById(params.id), + }; +} + +describe('wrapLoadWithSentry', () => { + beforeEach(() => { + mockCaptureException.mockClear(); + mockAddExceptionMechanism.mockClear(); + mockScope = new Scope(); + }); + + it('calls captureException', async () => { + const wrappedLoad = wrapLoadWithSentry(erroringLoad); + const res = wrappedLoad({ params: { id: '1' } } as any); + await expect(res).rejects.toThrow(); + + expect(mockCaptureException).toHaveBeenCalledTimes(1); + }); + + it('adds an exception mechanism', async () => { + const addEventProcessorSpy = jest.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { + void callback({}, { event_id: 'fake-event-id' }); + return mockScope; + }); + + const wrappedLoad = wrapLoadWithSentry(erroringLoad); + const res = wrappedLoad({ params: { id: '1' } } as any); + await expect(res).rejects.toThrow(); + + expect(addEventProcessorSpy).toBeCalledTimes(1); + expect(mockAddExceptionMechanism).toBeCalledTimes(1); + expect(mockAddExceptionMechanism).toBeCalledWith( + {}, + { handled: false, type: 'instrument', data: { function: 'load' } }, + ); + }); +}); From afddce30076b572019d532f1ba7e3e7fc03e1112 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 13 Mar 2023 17:17:03 +0100 Subject: [PATCH 2/9] add case for load --- packages/sveltekit/.eslintrc.js | 9 +++ packages/sveltekit/src/server/load.ts | 16 ++++- .../sveltekit/test/client/handleError.test.ts | 4 -- .../sveltekit/test/server/handleError.test.ts | 6 +- packages/sveltekit/test/server/load.test.ts | 66 ++++++++++++++----- 5 files changed, 73 insertions(+), 28 deletions(-) diff --git a/packages/sveltekit/.eslintrc.js b/packages/sveltekit/.eslintrc.js index 2d614f46733b..8fcc9ae58f9a 100644 --- a/packages/sveltekit/.eslintrc.js +++ b/packages/sveltekit/.eslintrc.js @@ -3,5 +3,14 @@ module.exports = { browser: true, node: true, }, + overrides: [ + { + files: ['*.ts'], + rules: { + // Turning this off because it's not working with @sveltejs/kit + 'import/no-unresolved': 'off', + }, + }, + ], extends: ['../../.eslintrc.js'], }; diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index c4fb0b71ec9a..e9882f675e97 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,7 +1,10 @@ import { captureException } from '@sentry/node'; import { addExceptionMechanism, objectify } from '@sentry/utils'; -// eslint-disable-next-line import/no-unresolved -import type { ServerLoad } from '@sveltejs/kit'; +import type { HttpError, ServerLoad } from '@sveltejs/kit'; + +function isHttpError(err: unknown): err is HttpError { + return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; +} /** * Wrap load function with Sentry @@ -16,7 +19,14 @@ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { } catch (e) { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can // store a seen flag on it. - const objectifiedErr = objectify(e); + const objectifiedErr = objectify(e) as unknown; + + // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error + // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they + // could be noisy. + if (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) { + throw objectifiedErr; + } captureException(objectifiedErr, scope => { scope.addEventProcessor(event => { diff --git a/packages/sveltekit/test/client/handleError.test.ts b/packages/sveltekit/test/client/handleError.test.ts index 6260d3dc4827..170322555308 100644 --- a/packages/sveltekit/test/client/handleError.test.ts +++ b/packages/sveltekit/test/client/handleError.test.ts @@ -1,8 +1,4 @@ import { Scope } from '@sentry/svelte'; -// For now disable the import/no-unresolved rule, because we don't have a way to -// tell eslint that we are only importing types from the @sveltejs/kit package without -// adding a custom resolver, which will take too much time. -// eslint-disable-next-line import/no-unresolved import type { HandleClientError, NavigationEvent } from '@sveltejs/kit'; import { vi } from 'vitest'; diff --git a/packages/sveltekit/test/server/handleError.test.ts b/packages/sveltekit/test/server/handleError.test.ts index d0a23667dad1..039964eeacef 100644 --- a/packages/sveltekit/test/server/handleError.test.ts +++ b/packages/sveltekit/test/server/handleError.test.ts @@ -1,8 +1,4 @@ import { Scope } from '@sentry/node'; -// For now disable the import/no-unresolved rule, because we don't have a way to -// tell eslint that we are only importing types from the @sveltejs/kit package without -// adding a custom resolver, which will take too much time. -// eslint-disable-next-line import/no-unresolved import type { HandleServerError, RequestEvent } from '@sveltejs/kit'; import { vi } from 'vitest'; @@ -80,6 +76,6 @@ describe('handleError', () => { expect(addEventProcessorSpy).toBeCalledTimes(1); expect(mockAddExceptionMechanism).toBeCalledTimes(1); - expect(mockAddExceptionMechanism).toBeCalledWith({}, { handled: false, type: 'sveltekit' }); + expect(mockAddExceptionMechanism).toBeCalledWith({}, { handled: false, type: 'instrument' }); }); }); diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index 04ed34d45a5d..59dd7611954a 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -1,14 +1,15 @@ import { Scope } from '@sentry/node'; -// eslint-disable-next-line import/no-unresolved import type { ServerLoad } from '@sveltejs/kit'; +import { error } from '@sveltejs/kit'; +import { vi } from 'vitest'; import { wrapLoadWithSentry } from '../../src/server/load'; -const mockCaptureException = jest.fn(); +const mockCaptureException = vi.fn(); let mockScope = new Scope(); -jest.mock('@sentry/node', () => { - const original = jest.requireActual('@sentry/node'); +vi.mock('@sentry/node', async () => { + const original = (await vi.importActual('@sentry/node')) as any; return { ...original, captureException: (err: unknown, cb: (arg0: unknown) => unknown) => { @@ -19,10 +20,10 @@ jest.mock('@sentry/node', () => { }; }); -const mockAddExceptionMechanism = jest.fn(); +const mockAddExceptionMechanism = vi.fn(); -jest.mock('@sentry/utils', () => { - const original = jest.requireActual('@sentry/utils'); +vi.mock('@sentry/utils', async () => { + const original = (await vi.importActual('@sentry/utils')) as any; return { ...original, addExceptionMechanism: (...args: unknown[]) => mockAddExceptionMechanism(...args), @@ -33,12 +34,6 @@ function getById(_id?: string) { throw new Error('error'); } -async function erroringLoad({ params }: Parameters[0]): Promise> { - return { - post: getById(params.id), - }; -} - describe('wrapLoadWithSentry', () => { beforeEach(() => { mockCaptureException.mockClear(); @@ -47,20 +42,59 @@ describe('wrapLoadWithSentry', () => { }); it('calls captureException', async () => { - const wrappedLoad = wrapLoadWithSentry(erroringLoad); + async function load({ params }: Parameters[0]): Promise> { + return { + post: getById(params.id), + }; + } + + const wrappedLoad = wrapLoadWithSentry(load); const res = wrappedLoad({ params: { id: '1' } } as any); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(1); }); + describe('with error() helper', () => { + it.each([ + // [statusCode, timesCalled] + [400, 0], + [401, 0], + [403, 0], + [404, 0], + [409, 0], + [429, 0], + [499, 0], + [500, 1], + [501, 1], + [503, 1], + [504, 1], + ])('error with status code %s calls captureException %s times', async (code, times) => { + async function load({ params }: Parameters[0]): Promise> { + throw error(code, params.id); + } + + const wrappedLoad = wrapLoadWithSentry(load); + const res = wrappedLoad({ params: { id: '1' } } as any); + await expect(res).rejects.toThrow(); + + expect(mockCaptureException).toHaveBeenCalledTimes(times); + }); + }); + it('adds an exception mechanism', async () => { - const addEventProcessorSpy = jest.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { + const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { void callback({}, { event_id: 'fake-event-id' }); return mockScope; }); - const wrappedLoad = wrapLoadWithSentry(erroringLoad); + async function load({ params }: Parameters[0]): Promise> { + return { + post: getById(params.id), + }; + } + + const wrappedLoad = wrapLoadWithSentry(load); const res = wrappedLoad({ params: { id: '1' } } as any); await expect(res).rejects.toThrow(); From ac43b2f9176111bb64fa5be329ab9fa5f5135983 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 13 Mar 2023 17:21:30 +0100 Subject: [PATCH 3/9] use sveltekit --- packages/sveltekit/src/client/handleError.ts | 2 +- packages/sveltekit/src/server/handleError.ts | 2 +- packages/sveltekit/src/server/load.ts | 2 +- packages/sveltekit/test/server/handleError.test.ts | 2 +- packages/sveltekit/test/server/load.test.ts | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/sveltekit/src/client/handleError.ts b/packages/sveltekit/src/client/handleError.ts index 8cc781c70686..5490a3e20980 100644 --- a/packages/sveltekit/src/client/handleError.ts +++ b/packages/sveltekit/src/client/handleError.ts @@ -16,7 +16,7 @@ export function handleErrorWithSentry(handleError?: HandleClientError): HandleCl captureException(input.error, scope => { scope.addEventProcessor(event => { addExceptionMechanism(event, { - type: 'instrument', + type: 'sveltekit', handled: false, }); return event; diff --git a/packages/sveltekit/src/server/handleError.ts b/packages/sveltekit/src/server/handleError.ts index 5dce61ec9bc4..449ca65e19ce 100644 --- a/packages/sveltekit/src/server/handleError.ts +++ b/packages/sveltekit/src/server/handleError.ts @@ -16,7 +16,7 @@ export function handleErrorWithSentry(handleError?: HandleServerError): HandleSe captureException(input.error, scope => { scope.addEventProcessor(event => { addExceptionMechanism(event, { - type: 'instrument', + type: 'sveltekit', handled: false, }); return event; diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index e9882f675e97..e2d20865a010 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -31,7 +31,7 @@ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { captureException(objectifiedErr, scope => { scope.addEventProcessor(event => { addExceptionMechanism(event, { - type: 'instrument', + type: 'sveltekit', handled: false, data: { function: 'load', diff --git a/packages/sveltekit/test/server/handleError.test.ts b/packages/sveltekit/test/server/handleError.test.ts index 039964eeacef..afe7f5cbe6df 100644 --- a/packages/sveltekit/test/server/handleError.test.ts +++ b/packages/sveltekit/test/server/handleError.test.ts @@ -76,6 +76,6 @@ describe('handleError', () => { expect(addEventProcessorSpy).toBeCalledTimes(1); expect(mockAddExceptionMechanism).toBeCalledTimes(1); - expect(mockAddExceptionMechanism).toBeCalledWith({}, { handled: false, type: 'instrument' }); + expect(mockAddExceptionMechanism).toBeCalledWith({}, { handled: false, type: 'sveltekit' }); }); }); diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index 59dd7611954a..ec2503b945c4 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -102,7 +102,7 @@ describe('wrapLoadWithSentry', () => { expect(mockAddExceptionMechanism).toBeCalledTimes(1); expect(mockAddExceptionMechanism).toBeCalledWith( {}, - { handled: false, type: 'instrument', data: { function: 'load' } }, + { handled: false, type: 'sveltekit', data: { function: 'load' } }, ); }); }); From a5a8fd652d538a44409fe4f10356a8c30bc39760 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 14 Mar 2023 17:12:16 +0100 Subject: [PATCH 4/9] make sure we are always returning original promise --- packages/sveltekit/src/server/load.ts | 72 ++++++++++++++++----------- 1 file changed, 43 insertions(+), 29 deletions(-) diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index e2d20865a010..169641abcae8 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,11 +1,41 @@ import { captureException } from '@sentry/node'; -import { addExceptionMechanism, objectify } from '@sentry/utils'; +import { addExceptionMechanism, objectify, isThenable } from '@sentry/utils'; import type { HttpError, ServerLoad } from '@sveltejs/kit'; function isHttpError(err: unknown): err is HttpError { return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; } +function captureAndThrowError(e: unknown): void { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. + const objectifiedErr = objectify(e); + + // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error + // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they + // could be noisy. + if (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) { + throw objectifiedErr; + } + + captureException(objectifiedErr, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'sveltekit', + handled: false, + data: { + function: 'load', + }, + }); + return event; + }); + + return scope; + }); + + throw objectifiedErr; +} + /** * Wrap load function with Sentry * @@ -13,38 +43,22 @@ function isHttpError(err: unknown): err is HttpError { */ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { return new Proxy(origLoad, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { + apply: (wrappingTarget, thisArg, args: Parameters) => { + let maybePromiseResult; + try { - return await wrappingTarget.apply(thisArg, args); + maybePromiseResult = wrappingTarget.apply(thisArg, args); } catch (e) { - // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can - // store a seen flag on it. - const objectifiedErr = objectify(e) as unknown; - - // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error - // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they - // could be noisy. - if (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) { - throw objectifiedErr; - } - - captureException(objectifiedErr, scope => { - scope.addEventProcessor(event => { - addExceptionMechanism(event, { - type: 'sveltekit', - handled: false, - data: { - function: 'load', - }, - }); - return event; - }); - - return scope; - }); + captureAndThrowError(e); + } - throw objectifiedErr; + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then(null, e => { + captureAndThrowError(e); + }); } + + return maybePromiseResult; }, }); } From f934239db73d5cbde5f593096f6696e55d6fa0ef Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 14 Mar 2023 17:19:15 +0100 Subject: [PATCH 5/9] fix lint --- packages/sveltekit/src/server/load.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index 169641abcae8..3bbfd9a276b6 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,5 +1,5 @@ import { captureException } from '@sentry/node'; -import { addExceptionMechanism, objectify, isThenable } from '@sentry/utils'; +import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; import type { HttpError, ServerLoad } from '@sveltejs/kit'; function isHttpError(err: unknown): err is HttpError { From 457ea90410731d9746437f08e592941eab0b3f21 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 14 Mar 2023 18:20:59 +0100 Subject: [PATCH 6/9] handle uncaught exceptions properly --- packages/sveltekit/src/server/load.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index 3bbfd9a276b6..ef0433091a9e 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -6,7 +6,7 @@ function isHttpError(err: unknown): err is HttpError { return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; } -function captureAndThrowError(e: unknown): void { +function sendErrorToSentry(e: unknown): unknown { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can // store a seen flag on it. const objectifiedErr = objectify(e); @@ -15,7 +15,7 @@ function captureAndThrowError(e: unknown): void { // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they // could be noisy. if (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) { - throw objectifiedErr; + return objectifiedErr; } captureException(objectifiedErr, scope => { @@ -33,7 +33,7 @@ function captureAndThrowError(e: unknown): void { return scope; }); - throw objectifiedErr; + return objectifiedErr; } /** @@ -49,12 +49,12 @@ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { try { maybePromiseResult = wrappingTarget.apply(thisArg, args); } catch (e) { - captureAndThrowError(e); + throw sendErrorToSentry(e); } if (isThenable(maybePromiseResult)) { Promise.resolve(maybePromiseResult).then(null, e => { - captureAndThrowError(e); + sendErrorToSentry(e); }); } From f9d4e8bdfb23c101d9e5e305398b29aefe12a897 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 13 Mar 2023 20:10:08 +0100 Subject: [PATCH 7/9] feat(sveltekit): Add wrapper for client load function --- packages/sveltekit/src/client/index.ts | 4 +- packages/sveltekit/src/client/load.ts | 39 ++++++++++ packages/sveltekit/src/index.types.ts | 4 +- packages/sveltekit/test/client/load.test.ts | 80 +++++++++++++++++++++ 4 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 packages/sveltekit/src/client/load.ts create mode 100644 packages/sveltekit/test/client/load.test.ts diff --git a/packages/sveltekit/src/client/index.ts b/packages/sveltekit/src/client/index.ts index dc6ad3407264..f60a353d8b1d 100644 --- a/packages/sveltekit/src/client/index.ts +++ b/packages/sveltekit/src/client/index.ts @@ -2,6 +2,4 @@ export * from '@sentry/svelte'; export { init } from './sdk'; export { handleErrorWithSentry } from './handleError'; - -// Just here so that eslint is happy until we export more stuff here -export const PLACEHOLDER_CLIENT = 'PLACEHOLDER'; +export { wrapLoadWithSentry } from './load'; diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts new file mode 100644 index 000000000000..636b56c532d7 --- /dev/null +++ b/packages/sveltekit/src/client/load.ts @@ -0,0 +1,39 @@ +import { captureException } from '@sentry/svelte'; +import { addExceptionMechanism, objectify } from '@sentry/utils'; +import type { ServerLoad } from '@sveltejs/kit'; + +/** + * Wrap load function with Sentry + * + * @param origLoad SvelteKit user defined load function + */ +export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { + return new Proxy(origLoad, { + apply: async (wrappingTarget, thisArg, args: Parameters) => { + try { + return await wrappingTarget.apply(thisArg, args); + } catch (e) { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. + const objectifiedErr = objectify(e) as unknown; + + captureException(objectifiedErr, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'sveltekit', + handled: false, + data: { + function: 'load', + }, + }); + return event; + }); + + return scope; + }); + + throw objectifiedErr; + } + }, + }); +} diff --git a/packages/sveltekit/src/index.types.ts b/packages/sveltekit/src/index.types.ts index 06ff806a8377..f332a526019f 100644 --- a/packages/sveltekit/src/index.types.ts +++ b/packages/sveltekit/src/index.types.ts @@ -9,7 +9,7 @@ export * from './server'; import type { Integration, Options, StackParser } from '@sentry/types'; // eslint-disable-next-line import/no-unresolved -import type { HandleClientError, HandleServerError } from '@sveltejs/kit'; +import type { HandleClientError, HandleServerError, ServerLoad } from '@sveltejs/kit'; import type * as clientSdk from './client'; import type * as serverSdk from './server'; @@ -21,6 +21,8 @@ export declare function handleErrorWithSentry; +export declare function wrapLoadWithSentry(origLoad: S): S; + // We export a merged Integrations object so that users can (at least typing-wise) use all integrations everywhere. export declare const Integrations: typeof clientSdk.Integrations & typeof serverSdk.Integrations; diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts new file mode 100644 index 000000000000..7cbfd3593c03 --- /dev/null +++ b/packages/sveltekit/test/client/load.test.ts @@ -0,0 +1,80 @@ +import { Scope } from '@sentry/svelte'; +import type { ServerLoad } from '@sveltejs/kit'; +import { vi } from 'vitest'; + +import { wrapLoadWithSentry } from '../../src/client/load'; + +const mockCaptureException = vi.fn(); +let mockScope = new Scope(); + +vi.mock('@sentry/svelte', async () => { + const original = (await vi.importActual('@sentry/svelte')) as any; + return { + ...original, + captureException: (err: unknown, cb: (arg0: unknown) => unknown) => { + cb(mockScope); + mockCaptureException(err, cb); + return original.captureException(err, cb); + }, + }; +}); + +const mockAddExceptionMechanism = vi.fn(); + +vi.mock('@sentry/utils', async () => { + const original = (await vi.importActual('@sentry/utils')) as any; + return { + ...original, + addExceptionMechanism: (...args: unknown[]) => mockAddExceptionMechanism(...args), + }; +}); + +function getById(_id?: string) { + throw new Error('error'); +} + +describe('wrapLoadWithSentry', () => { + beforeEach(() => { + mockCaptureException.mockClear(); + mockAddExceptionMechanism.mockClear(); + mockScope = new Scope(); + }); + + it('calls captureException', async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: getById(params.id), + }; + } + + const wrappedLoad = wrapLoadWithSentry(load); + const res = wrappedLoad({ params: { id: '1' } } as any); + await expect(res).rejects.toThrow(); + + expect(mockCaptureException).toHaveBeenCalledTimes(1); + }); + + it('adds an exception mechanism', async () => { + const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { + void callback({}, { event_id: 'fake-event-id' }); + return mockScope; + }); + + async function load({ params }: Parameters[0]): Promise> { + return { + post: getById(params.id), + }; + } + + const wrappedLoad = wrapLoadWithSentry(load); + const res = wrappedLoad({ params: { id: '1' } } as any); + await expect(res).rejects.toThrow(); + + expect(addEventProcessorSpy).toBeCalledTimes(1); + expect(mockAddExceptionMechanism).toBeCalledTimes(1); + expect(mockAddExceptionMechanism).toBeCalledWith( + {}, + { handled: false, type: 'sveltekit', data: { function: 'load' } }, + ); + }); +}); From 3042fe148fa07a00d39d5825b4622eec6169e61a Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 14 Mar 2023 17:20:20 +0100 Subject: [PATCH 8/9] client: return promise explicitly --- packages/sveltekit/src/client/load.ts | 58 +++++++++++++++++---------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index 636b56c532d7..892d9594b957 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,7 +1,30 @@ import { captureException } from '@sentry/svelte'; -import { addExceptionMechanism, objectify } from '@sentry/utils'; +import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; import type { ServerLoad } from '@sveltejs/kit'; +function captureAndThrowError(e: unknown): void { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. + const objectifiedErr = objectify(e); + + captureException(objectifiedErr, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'sveltekit', + handled: false, + data: { + function: 'load', + }, + }); + return event; + }); + + return scope; + }); + + throw objectifiedErr; +} + /** * Wrap load function with Sentry * @@ -9,31 +32,22 @@ import type { ServerLoad } from '@sveltejs/kit'; */ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { return new Proxy(origLoad, { - apply: async (wrappingTarget, thisArg, args: Parameters) => { + apply: (wrappingTarget, thisArg, args: Parameters) => { + let maybePromiseResult; + try { - return await wrappingTarget.apply(thisArg, args); + maybePromiseResult = wrappingTarget.apply(thisArg, args); } catch (e) { - // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can - // store a seen flag on it. - const objectifiedErr = objectify(e) as unknown; - - captureException(objectifiedErr, scope => { - scope.addEventProcessor(event => { - addExceptionMechanism(event, { - type: 'sveltekit', - handled: false, - data: { - function: 'load', - }, - }); - return event; - }); - - return scope; - }); + captureAndThrowError(e); + } - throw objectifiedErr; + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then(null, e => { + captureAndThrowError(e); + }); } + + return maybePromiseResult; }, }); } From 25c9a255ae071693f3f6c388d540f62f8fad2f90 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 14 Mar 2023 18:33:33 +0100 Subject: [PATCH 9/9] update client code --- packages/sveltekit/src/client/load.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index 892d9594b957..fbaa5f98799f 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -2,7 +2,7 @@ import { captureException } from '@sentry/svelte'; import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; import type { ServerLoad } from '@sveltejs/kit'; -function captureAndThrowError(e: unknown): void { +function sendErrorToSentry(e: unknown): unknown { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can // store a seen flag on it. const objectifiedErr = objectify(e); @@ -22,7 +22,7 @@ function captureAndThrowError(e: unknown): void { return scope; }); - throw objectifiedErr; + return objectifiedErr; } /** @@ -38,12 +38,12 @@ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { try { maybePromiseResult = wrappingTarget.apply(thisArg, args); } catch (e) { - captureAndThrowError(e); + throw sendErrorToSentry(e); } if (isThenable(maybePromiseResult)) { Promise.resolve(maybePromiseResult).then(null, e => { - captureAndThrowError(e); + sendErrorToSentry(e); }); }