fix(sveltekit): Log error to console by default in handleErrorWithSentry (#7674)

Previously, our `handleErrorWithSentry` wrapper did nothing if users didn't provide a custom error handler as an argument. While IMO it's good that the error handler is an optional parameter, this swallowed errors and didn't log them to the console anymore. 

This patch fixes that by adding a default error handler which our wrapper invokes if no custom handler was provided. The client- and server default handlers log the error to the console, exactly like the default handlers by SvelteKit ([client](https://github.com/sveltejs/kit/blob/369e7d6851f543a40c947e033bfc4a9506fdc0a8/packages/kit/src/core/sync/write_client_manifest.js#LL127C2-L127C2), [server](https://github.com/sveltejs/kit/blob/369e7d6851f543a40c947e033bfc4a9506fdc0a8/packages/kit/src/runtime/server/index.js#L43)).

Author: Lms24

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<HandleClientError>[0]): ReturnType<HandleClientError> {
+  // 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<HandleClientError> => {
     captureException(input.error, scope => {
       scope.addEventProcessor(event => {
@@ -23,8 +30,7 @@ export function handleErrorWithSentry(handleError?: HandleClientError): HandleCl
       });
       return scope;
     });
-    if (handleError) {
-      return handleError(input);
-    }
+
+    return handleError(input);
   };
 }

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<HandleServerError>[0]): ReturnType<HandleServerError> {
+  // @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<HandleServerError> => {
     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);
   };
 }

packages/sveltekit/test/client/handleError.test.ts

@@ -45,31 +45,40 @@ 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 () => {
-    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));
-  });
+      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: navigationEvent })) as any;
+    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;
 
-    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 () => {

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-provided 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 () => {