@sentry/remix: Incompatible type in `wrapRemixHandleError`

[haines]

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).

[Lms24]

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

[onurtemizkan]

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);
	});
	}

[haines]

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 🤷‍♂️