ref(sveltekit): Improve SvelteKit 2.0 404 server error handling (#9901)

Improve our logic to filter out "Not Found" errors in our
server-side `handleError` hook wrapper:

- We now use SvelteKit 2.0 - native [error
properties](https://kit.svelte.dev/docs/migrating-to-sveltekit-2#improved-error-handling)
(`status` ~and `message`~) to check for "Not Found" errors
- Adjusted types for type safety and backwards compatibility with
SvelteKit 1.x where the ~two properties~ property don't exist.
- Updated Sveltekit to 2.0 in our dev dependency to work with latest
types.

Author: Lms24

packages/sveltekit/package.json

@@ -42,10 +42,10 @@
     "sorcery": "0.11.0"
   },
   "devDependencies": {
-    "@sveltejs/kit": "^1.11.0",
+    "@sveltejs/kit": "^2.0.2",
     "rollup": "^3.20.2",
-    "svelte": "^3.44.0",
-    "vite": "4.0.5"
+    "svelte": "^4.2.8",
+    "vite": "^5.0.10"
   },
   "scripts": {
     "build": "run-p build:transpile build:types",

packages/sveltekit/src/client/handleError.ts

@@ -11,13 +11,16 @@ function defaultErrorHandler({ error }: Parameters<HandleClientError>[0]): Retur
   });
 }
 
+// TODO: add backwards-compatible type for kit 1.x (soon)
+type HandleClientErrorInput = Parameters<HandleClientError>[0];
+
 /**
  * Wrapper for the SvelteKit error handler that sends the error to Sentry.
  *
  * @param handleError The original SvelteKit error handler.
  */
 export function handleErrorWithSentry(handleError: HandleClientError = defaultErrorHandler): HandleClientError {
-  return (input: { error: unknown; event: NavigationEvent }): ReturnType<HandleClientError> => {
+  return (input: HandleClientErrorInput): ReturnType<HandleClientError> => {
     captureException(input.error, {
       mechanism: {
         type: 'sveltekit',

packages/sveltekit/src/server/handleError.ts

@@ -1,5 +1,5 @@
 import { captureException } from '@sentry/node';
-import type { HandleServerError, RequestEvent } from '@sveltejs/kit';
+import type { HandleServerError } from '@sveltejs/kit';
 
 import { flushIfServerless } from './utils';
 
@@ -11,14 +11,28 @@ function defaultErrorHandler({ error }: Parameters<HandleServerError>[0]): Retur
   console.error(error && error.stack);
 }
 
+type HandleServerErrorInput = Parameters<HandleServerError>[0];
+
+/**
+ * Backwards-compatible HandleServerError Input type for SvelteKit 1.x and 2.x
+ * `message` and `status` were added in 2.x.
+ * For backwards-compatibility, we make them optional
+ *
+ * @see https://kit.svelte.dev/docs/migrating-to-sveltekit-2#improved-error-handling
+ */
+type SafeHandleServerErrorInput = Omit<HandleServerErrorInput, 'status' | 'message'> &
+  Partial<Pick<HandleServerErrorInput, 'status' | 'message'>>;
+
 /**
  * Wrapper for the SvelteKit error handler that sends the error to Sentry.
  *
  * @param handleError The original SvelteKit error handler.
  */
 export function handleErrorWithSentry(handleError: HandleServerError = defaultErrorHandler): HandleServerError {
-  return async (input: { error: unknown; event: RequestEvent }): Promise<void | App.Error> => {
+  return async (input: SafeHandleServerErrorInput): Promise<void | App.Error> => {
     if (isNotFoundError(input)) {
+      // We're extra cautious with SafeHandleServerErrorInput - this type is not compatible with HandleServerErrorInput
+      // @ts-expect-error - we're still passing the same object, just with a different (backwards-compatible) type
       return handleError(input);
     }
 
@@ -31,19 +45,26 @@ export function handleErrorWithSentry(handleError: HandleServerError = defaultEr
 
     await flushIfServerless();
 
+    // We're extra cautious with SafeHandleServerErrorInput - this type is not compatible with HandleServerErrorInput
+    // @ts-expect-error - we're still passing the same object, just with a different (backwards-compatible) type
     return handleError(input);
   };
 }
 
 /**
  * When a page request fails because the page is not found, SvelteKit throws a "Not found" error.
- * In the error handler here, we can't access the response yet (which we do in the load instrumentation),
- * so we have to check if the error is a "Not found" error by checking if the route id is missing and
- * by checking the error message on top of the raw stack trace.
  */
-function isNotFoundError(input: { error: unknown; event: RequestEvent }): boolean {
-  const { error, event } = input;
+function isNotFoundError(input: SafeHandleServerErrorInput): boolean {
+  const { error, event, status } = input;
+
+  // SvelteKit 2.0 offers a reliable way to check for a Not Found error:
+  if (status === 404) {
+    return true;
+  }
 
+  // SvelteKit 1.x doesn't offer a reliable way to check for a Not Found error.
+  // So we check the route id (shouldn't exist) and the raw stack trace
+  // We can delete all of this below whenever we drop Kit 1.x support
   const hasNoRouteId = !event.route || !event.route.id;
 
   const rawStack: string =

packages/sveltekit/test/common/utils.test.ts

@@ -34,7 +34,7 @@ describe('isHttpError', () => {
     expect(isHttpError(httpErrorObject)).toBe(true);
   });
 
-  it.each([new Error(), redirect(301, '/users/id'), 'string error', { status: 404 }, { body: 'Not found' }])(
+  it.each([new Error(), { status: 301, message: '/users/id' }, 'string error', { status: 404 }, { body: 'Not found' }])(
     'returns `false` for other thrown objects (%s)',
     httpErrorObject => {
       expect(isHttpError(httpErrorObject)).toBe(false);

packages/sveltekit/test/server/handleError.test.ts

@@ -26,7 +26,7 @@ describe('handleError', () => {
     consoleErrorSpy.mockClear();
   });
 
-  it('doesn\'t capture "Not found" errors for incorrect navigations', async () => {
+  it('doesn\'t capture "Not found" errors for incorrect navigations [Kit 1.x]', async () => {
     const wrappedHandleError = handleErrorWithSentry();
     const mockError = new Error('Not found: /asdf/123');
     const mockEvent = {
@@ -35,18 +35,39 @@ describe('handleError', () => {
       // ...
     } as RequestEvent;
 
+    // @ts-expect-error - purposefully omitting status and message to cover SvelteKit 1.x compatibility
     const returnVal = await wrappedHandleError({ error: mockError, event: mockEvent });
 
     expect(returnVal).not.toBeDefined();
     expect(mockCaptureException).toHaveBeenCalledTimes(0);
     expect(consoleErrorSpy).toHaveBeenCalledTimes(1);
   });
 
+  it('doesn\'t capture "Not found" errors for incorrect navigations [Kit 2.x]', async () => {
+    const wrappedHandleError = handleErrorWithSentry();
+
+    const returnVal = await wrappedHandleError({
+      error: new Error('404 /asdf/123'),
+      event: requestEvent,
+      status: 404,
+      message: 'Not Found',
+    });
+
+    expect(returnVal).not.toBeDefined();
+    expect(mockCaptureException).toHaveBeenCalledTimes(0);
+    expect(consoleErrorSpy).toHaveBeenCalledTimes(1);
+  });
+
   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 });
+      const returnVal = await wrappedHandleError({
+        error: mockError,
+        event: requestEvent,
+        status: 500,
+        message: 'Internal Error',
+      });
 
       expect(returnVal).not.toBeDefined();
       expect(mockCaptureException).toHaveBeenCalledTimes(1);
@@ -58,7 +79,12 @@ describe('handleError', () => {
     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;
+      const returnVal = (await wrappedHandleError({
+        error: mockError,
+        event: requestEvent,
+        status: 500,
+        message: 'Internal Error',
+      })) as any;
 
       expect(returnVal.message).toEqual('Whoops!');
       expect(mockCaptureException).toHaveBeenCalledTimes(1);