@sentry/remix: Incompatible type in wrapRemixHandleError

Is there an existing issue for this?

I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
I have reviewed the documentation https://docs.sentry.io/
I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/remix

SDK Version

7.93.0

Framework Version

2.5.0

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

https://github.com/haines/sentry-remix-type-error

Instead of doing

export const handleError = wrapRemixHandleError;

attempt to call wrapRemixHandleError within an existing handleError method

export function handleError(
  error: unknown,
  args: ActionFunctionArgs | LoaderFunctionArgs,
): void {
  wrapRemixHandleError(error, args);
  // ...
}

Expected Result

Code type-checks successfully.

Actual Result

TypeScript reports an error

error TS2345: Argument of type 'ActionFunctionArgs | LoaderFunctionArgs' is not assignable to parameter of type 'DataFunctionArgs'.
  Type 'ActionFunctionArgs' is not assignable to type 'DataFunctionArgs'.
    Types of property 'request' are incompatible.
      Type 'Request' is not assignable to type 'RemixRequest'.
        Type 'Request' is not assignable to type 'Record<string | symbol, RemixRequestState>'.
          Index signature for type 'string' is missing in type 'Request'.

(the error is the same if you use the deprecated DataFunctionArgs type from Remix, so it's not just because of the change to use ActionFunctionArgs | LoaderFunctionArgs).

@haines

Hi @haines thanks for writing in! We'll take a look.
cc @onurtemizkan when you have time, please

@haines

Hi @haines, thanks for reporting.

We have exported wrapRemixHandleError for easy instrumentation for non-custom error-handling scenarios.

Should be used like:
export const handleError = wrapRemixHandleError;

If you would like to have custom error handling logic inside handleError, I'd recommend using Sentry.captureRemixServerException instead, which wrapRemixHandleError uses in its core:

sentry-javascript/packages/remix/src/utils/instrumentServer.ts

Lines 91 to 105 in 7e3207d

    
           export function wrapRemixHandleError(err: unknown, { request }: DataFunctionArgs): void { 
        
             // We are skipping thrown responses here as they are handled by 
        
             // `captureRemixServerException` at loader / action level 
        
             // We don't want to capture them twice. 
        
             // This function is only for capturing unhandled server-side exceptions. 
        
             // https://remix.run/docs/en/main/file-conventions/entry.server#thrown-responses 
        
             // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders 
        
             if (isResponse(err) || isRouteErrorResponse(err)) { 
        
               return; 
        
             } 
        
             captureRemixServerException(err, 'remix.server.handleError', request).then(null, e => { 
        
               DEBUG_BUILD && logger.warn('Failed to capture Remix Server exception.', e); 
        
             }); 
        
           }

@onurtemizkan

Hi @onurtemizkan, thanks for looking at this.

To use captureRemixServerException directly, I would have to reproduce the logic of wrapRemixHandleError within my handleError implementation to avoid capturing thrown responses twice, and remember to catch the result so that failure doesn't result in an unhandled promise rejection.

export function handleError(
  error: unknown,
  { request }: ActionFunctionArgs | LoaderFunctionArgs,
): void {
  if (!isResponse(err) && !isRouteErrorResponse(err)) {
    captureRemixServerException(error, 'remix.server.handleError', request).catch(() => {});
  }

  // ...
}

Yes, it's doable, but it'd be nicer to just be able to call wrapRemixHandleError (and then I don't have to keep track of any upstream changes made in @sentry/remix that I might need to copy). It doesn't really make sense to me that a function would only be able to be used if exported directly 🤷‍♂️

@onurtemizkan

@onurtemizkan It would make sense to me that we provided a wrapper for user's handleError function. Do we already have that?

@lforst

@lforst, we don't have a wrapper, but we can make this work as @haines mentioned. If we update the typing of the implementation.

export function handleError(
  error: unknown,
  args: ActionFunctionArgs | LoaderFunctionArgs,
): void {
  wrapRemixHandleError(error, args);
  // ...
}

We can make this loosely typed to reduce the risk of breaking when Remix changes its internal types.

I would argue we should have a wrapper that takes a users implementation of the handleError function.

The type definition should looks like this:

function wrapHandleErrorWithSentry<H extends ((...args: any[]) => any)>(errorHandler: H): H;

Similar to how we do it in Next.js:

sentry-javascript/packages/nextjs/src/index.types.ts

Lines 177 to 183 in ef2834d

    
           /** 
        
            * Wraps an `app` directory server component with Sentry error and performance instrumentation. 
        
            */ 
        
           export declare function wrapServerComponentWithSentry<F extends (...args: any[]) => any>( 
        
             WrappingTarget: F, 
        
             context: ServerComponentContext, 
        
           ): F;

We then need to do some internal validation of the parameters so that the arguments are what we expect them to be.

This makes sense to me as well. So are we going to replace the current implementation wrapRemixHandleError with the new wrapper, or are we going to keep both (if so we may need to rename one to avoid confusion)?

I would add a new function called wrapHandleErrorWithSentry like I suggested and leave the old one exactly as it is. Naming-wise I recommend we alias wrapRemixHandleError with sentryHandleError, because it is not really wrapping things but instead a full handler. We could deprecate the old name (wrapRemixHandleError).

Sounds good 👍 I'll try to open a PR later today.

Update manual-setup.mdx

Update manual-setup.mdx (#10393)

Navigation Menu

Repository navigation

@sentry/remix: Incompatible type in `wrapRemixHandleError` #10212

Is there an existing issue for this?

How do you use Sentry?

Which SDK are you using?

SDK Version

Framework Version

Link to Sentry event

SDK Setup

Steps to Reproduce

Expected Result

Actual Result

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	export function wrapRemixHandleError(err: unknown, { request }: DataFunctionArgs): void {
	// We are skipping thrown responses here as they are handled by
	// `captureRemixServerException` at loader / action level
	// We don't want to capture them twice.
	// This function is only for capturing unhandled server-side exceptions.
	// https://remix.run/docs/en/main/file-conventions/entry.server#thrown-responses
	// https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders
	if (isResponse(err) \|\| isRouteErrorResponse(err)) {
	return;
	}

	captureRemixServerException(err, 'remix.server.handleError', request).then(null, e => {
	DEBUG_BUILD && logger.warn('Failed to capture Remix Server exception.', e);
	});
	}

	/**
	* Wraps an `app` directory server component with Sentry error and performance instrumentation.
	*/
	export declare function wrapServerComponentWithSentry<F extends (...args: any[]) => any>(
	WrappingTarget: F,
	context: ServerComponentContext,
	): F;

@sentry/remix: Incompatible type in wrapRemixHandleError #10212

Description

Is there an existing issue for this?

How do you use Sentry?

Which SDK are you using?

SDK Version

Framework Version

Link to Sentry event

SDK Setup

Steps to Reproduce

Expected Result

Actual Result

Activity

Lms24 commented on Jan 17, 2024

onurtemizkan commented on Jan 22, 2024

haines commented on Jan 22, 2024

lforst commented on Jan 25, 2024

onurtemizkan commented on Jan 25, 2024

lforst commented on Jan 25, 2024

onurtemizkan commented on Jan 25, 2024

lforst commented on Jan 25, 2024

onurtemizkan commented on Jan 25, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

@sentry/remix: Incompatible type in `wrapRemixHandleError` #10212