From cb3b6e5e0b5ab285bb6ef82b512b97788a45297a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 30 Mar 2023 15:32:13 +0200 Subject: [PATCH 1/3] fix(sveltekit): Log console to error by default in `handleErrorWithSentry` --- packages/sveltekit/src/client/handleError.ts | 9 ++++++++- packages/sveltekit/test/client/handleError.test.ts | 7 ++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/sveltekit/src/client/handleError.ts b/packages/sveltekit/src/client/handleError.ts index 5490a3e20980..e815cec3d07a 100644 --- a/packages/sveltekit/src/client/handleError.ts +++ b/packages/sveltekit/src/client/handleError.ts @@ -6,12 +6,19 @@ import { addExceptionMechanism } from '@sentry/utils'; // eslint-disable-next-line import/no-unresolved import type { HandleClientError, NavigationEvent } from '@sveltejs/kit'; +// The SvelteKit default error handler just logs the error to the console +// see: https://github.com/sveltejs/kit/blob/369e7d6851f543a40c947e033bfc4a9506fdc0a8/packages/kit/src/core/sync/write_client_manifest.js#LL127C2-L127C2 +function defaultErrorHandler({ error }: Parameters[0]): ReturnType { + // eslint-disable-next-line no-console + console.error(error); +} + /** * Wrapper for the SvelteKit error handler that sends the error to Sentry. * * @param handleError The original SvelteKit error handler. */ -export function handleErrorWithSentry(handleError?: HandleClientError): HandleClientError { +export function handleErrorWithSentry(handleError: HandleClientError = defaultErrorHandler): HandleClientError { return (input: { error: unknown; event: NavigationEvent }): ReturnType => { captureException(input.error, scope => { scope.addEventProcessor(event => { diff --git a/packages/sveltekit/test/client/handleError.test.ts b/packages/sveltekit/test/client/handleError.test.ts index 170322555308..cbdd8423d1e6 100644 --- a/packages/sveltekit/test/client/handleError.test.ts +++ b/packages/sveltekit/test/client/handleError.test.ts @@ -45,14 +45,17 @@ const navigationEvent: NavigationEvent = { url: new URL('http://example.org/users/123'), }; +const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(_ => {}); + describe('handleError', () => { beforeEach(() => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); + consoleErrorSpy.mockClear(); mockScope = new Scope(); }); - it('works when a handleError func is not provided', async () => { + it('falls back to default handler when handleError func is not provided', async () => { const wrappedHandleError = handleErrorWithSentry(); const mockError = new Error('test'); const returnVal = await wrappedHandleError({ error: mockError, event: navigationEvent }); @@ -60,6 +63,8 @@ describe('handleError', () => { expect(returnVal).not.toBeDefined(); expect(mockCaptureException).toHaveBeenCalledTimes(1); expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); + // The default handler logs the error to the console + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); }); it('calls captureException', async () => { From 5afd8cccd45cfb2335bd7e4ad9afc40b18bd1e32 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 30 Mar 2023 15:46:06 +0200 Subject: [PATCH 2/3] add server-side fallback --- packages/sveltekit/src/client/handleError.ts | 5 +-- packages/sveltekit/src/server/handleError.ts | 15 +++++-- .../sveltekit/test/client/handleError.test.ts | 40 ++++++++++--------- .../sveltekit/test/server/handleError.test.ts | 39 +++++++++++------- 4 files changed, 59 insertions(+), 40 deletions(-) diff --git a/packages/sveltekit/src/client/handleError.ts b/packages/sveltekit/src/client/handleError.ts index e815cec3d07a..5b24c2ef9357 100644 --- a/packages/sveltekit/src/client/handleError.ts +++ b/packages/sveltekit/src/client/handleError.ts @@ -30,8 +30,7 @@ export function handleErrorWithSentry(handleError: HandleClientError = defaultEr }); return scope; }); - if (handleError) { - return handleError(input); - } + + return handleError(input); }; } diff --git a/packages/sveltekit/src/server/handleError.ts b/packages/sveltekit/src/server/handleError.ts index 449ca65e19ce..4d886fc80224 100644 --- a/packages/sveltekit/src/server/handleError.ts +++ b/packages/sveltekit/src/server/handleError.ts @@ -6,12 +6,20 @@ import { addExceptionMechanism } from '@sentry/utils'; // eslint-disable-next-line import/no-unresolved import type { HandleServerError, RequestEvent } from '@sveltejs/kit'; +// The SvelteKit default error handler just logs the error's stack trace to the console +// see: https://github.com/sveltejs/kit/blob/369e7d6851f543a40c947e033bfc4a9506fdc0a8/packages/kit/src/runtime/server/index.js#L43 +function defaultErrorHandler({ error }: Parameters[0]): ReturnType { + // @ts-expect-error this conforms to the default implementation (including this ts-expect-error) + // eslint-disable-next-line no-console + console.error(error && error.stack); +} + /** * Wrapper for the SvelteKit error handler that sends the error to Sentry. * * @param handleError The original SvelteKit error handler. */ -export function handleErrorWithSentry(handleError?: HandleServerError): HandleServerError { +export function handleErrorWithSentry(handleError: HandleServerError = defaultErrorHandler): HandleServerError { return (input: { error: unknown; event: RequestEvent }): ReturnType => { captureException(input.error, scope => { scope.addEventProcessor(event => { @@ -23,8 +31,7 @@ export function handleErrorWithSentry(handleError?: HandleServerError): HandleSe }); return scope; }); - if (handleError) { - return handleError(input); - } + + return handleError(input); }; } diff --git a/packages/sveltekit/test/client/handleError.test.ts b/packages/sveltekit/test/client/handleError.test.ts index cbdd8423d1e6..21f25355c137 100644 --- a/packages/sveltekit/test/client/handleError.test.ts +++ b/packages/sveltekit/test/client/handleError.test.ts @@ -55,26 +55,30 @@ describe('handleError', () => { mockScope = new Scope(); }); - it('falls back to default handler when handleError func is not provided', async () => { - const wrappedHandleError = handleErrorWithSentry(); - const mockError = new Error('test'); - const returnVal = await wrappedHandleError({ error: mockError, event: navigationEvent }); + describe('calls captureException', () => { + it('invokes the default handler if no handleError func is provided', async () => { + const wrappedHandleError = handleErrorWithSentry(); + const mockError = new Error('test'); + const returnVal = await wrappedHandleError({ error: mockError, event: navigationEvent }); + + expect(returnVal).not.toBeDefined(); + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); + // The default handler logs the error to the console + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + }); - expect(returnVal).not.toBeDefined(); - expect(mockCaptureException).toHaveBeenCalledTimes(1); - expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); - // The default handler logs the error to the console - expect(consoleErrorSpy).toHaveBeenCalledTimes(1); - }); + it('invokes the user-procided error handler', async () => { + const wrappedHandleError = handleErrorWithSentry(handleError); + const mockError = new Error('test'); + const returnVal = (await wrappedHandleError({ error: mockError, event: navigationEvent })) as any; - it('calls captureException', async () => { - const wrappedHandleError = handleErrorWithSentry(handleError); - const mockError = new Error('test'); - const returnVal = (await wrappedHandleError({ error: mockError, event: navigationEvent })) as any; - - expect(returnVal.message).toEqual('Whoops!'); - expect(mockCaptureException).toHaveBeenCalledTimes(1); - expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); + expect(returnVal.message).toEqual('Whoops!'); + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); + // Check that the default handler wasn't invoked + expect(consoleErrorSpy).toHaveBeenCalledTimes(0); + }); }); it('adds an exception mechanism', async () => { diff --git a/packages/sveltekit/test/server/handleError.test.ts b/packages/sveltekit/test/server/handleError.test.ts index afe7f5cbe6df..f2bd8da9eb1b 100644 --- a/packages/sveltekit/test/server/handleError.test.ts +++ b/packages/sveltekit/test/server/handleError.test.ts @@ -37,31 +37,40 @@ function handleError(_input: { error: unknown; event: RequestEvent }): ReturnTyp const requestEvent = {} as RequestEvent; +const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(_ => {}); + describe('handleError', () => { beforeEach(() => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); + consoleErrorSpy.mockClear(); mockScope = new Scope(); }); - it('works when a handleError func is not provided', async () => { - const wrappedHandleError = handleErrorWithSentry(); - const mockError = new Error('test'); - const returnVal = await wrappedHandleError({ error: mockError, event: requestEvent }); + describe('calls captureException', () => { + it('invokes the default handler if no handleError func is provided', async () => { + const wrappedHandleError = handleErrorWithSentry(); + const mockError = new Error('test'); + const returnVal = await wrappedHandleError({ error: mockError, event: requestEvent }); - expect(returnVal).not.toBeDefined(); - expect(mockCaptureException).toHaveBeenCalledTimes(1); - expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); - }); + expect(returnVal).not.toBeDefined(); + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); + // The default handler logs the error to the console + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + }); - it('calls captureException', async () => { - const wrappedHandleError = handleErrorWithSentry(handleError); - const mockError = new Error('test'); - const returnVal = (await wrappedHandleError({ error: mockError, event: requestEvent })) as any; + it('invokes the user-procided error handler', async () => { + const wrappedHandleError = handleErrorWithSentry(handleError); + const mockError = new Error('test'); + const returnVal = (await wrappedHandleError({ error: mockError, event: requestEvent })) as any; - expect(returnVal.message).toEqual('Whoops!'); - expect(mockCaptureException).toHaveBeenCalledTimes(1); - expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); + expect(returnVal.message).toEqual('Whoops!'); + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, expect.any(Function)); + // Check that the default handler wasn't invoked + expect(consoleErrorSpy).toHaveBeenCalledTimes(0); + }); }); it('adds an exception mechanism', async () => { From 0ecac4e2547c92e7e2d3c87c3c8768fef09f1ba2 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 30 Mar 2023 15:52:42 +0200 Subject: [PATCH 3/3] fix typo --- packages/sveltekit/test/client/handleError.test.ts | 2 +- packages/sveltekit/test/server/handleError.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/sveltekit/test/client/handleError.test.ts b/packages/sveltekit/test/client/handleError.test.ts index 21f25355c137..4dc2e6658af5 100644 --- a/packages/sveltekit/test/client/handleError.test.ts +++ b/packages/sveltekit/test/client/handleError.test.ts @@ -68,7 +68,7 @@ describe('handleError', () => { expect(consoleErrorSpy).toHaveBeenCalledTimes(1); }); - it('invokes the user-procided error handler', async () => { + it('invokes the user-provided error handler', async () => { const wrappedHandleError = handleErrorWithSentry(handleError); const mockError = new Error('test'); const returnVal = (await wrappedHandleError({ error: mockError, event: navigationEvent })) as any; diff --git a/packages/sveltekit/test/server/handleError.test.ts b/packages/sveltekit/test/server/handleError.test.ts index f2bd8da9eb1b..e44d5553f928 100644 --- a/packages/sveltekit/test/server/handleError.test.ts +++ b/packages/sveltekit/test/server/handleError.test.ts @@ -60,7 +60,7 @@ describe('handleError', () => { expect(consoleErrorSpy).toHaveBeenCalledTimes(1); }); - it('invokes the user-procided error handler', async () => { + it('invokes the user-provided error handler', async () => { const wrappedHandleError = handleErrorWithSentry(handleError); const mockError = new Error('test'); const returnVal = (await wrappedHandleError({ error: mockError, event: requestEvent })) as any;