From 8c7636e0a920dda1e184791c19f76ad744946d20 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 28 Jun 2023 11:41:42 +0100 Subject: [PATCH] feat(remix): Add v2 support. --- packages/remix/src/client/errors.tsx | 65 +++++++++++++++++++ .../client.tsx => client/performance.tsx} | 5 +- packages/remix/src/index.client.tsx | 3 +- packages/remix/src/index.server.ts | 4 +- packages/remix/src/utils/futureFlags.ts | 35 ++++++++++ packages/remix/src/utils/instrumentServer.ts | 26 +++++++- packages/remix/src/utils/types.ts | 37 +++++++++++ .../test/integration/app_v2/entry.server.tsx | 10 ++- .../remix/test/integration/app_v2/root.tsx | 13 +++- .../routes/action-json-response.$id.tsx | 4 +- .../routes/loader-json-response.$id.tsx | 2 +- .../test/client/errorboundary.test.ts | 16 +++-- .../integration/test/server/action.test.ts | 22 ++++--- .../integration/test/server/loader.test.ts | 8 +-- 14 files changed, 220 insertions(+), 30 deletions(-) create mode 100644 packages/remix/src/client/errors.tsx rename packages/remix/src/{performance/client.tsx => client/performance.tsx} (95%) create mode 100644 packages/remix/src/utils/futureFlags.ts diff --git a/packages/remix/src/client/errors.tsx b/packages/remix/src/client/errors.tsx new file mode 100644 index 000000000000..9e4d1d1c745a --- /dev/null +++ b/packages/remix/src/client/errors.tsx @@ -0,0 +1,65 @@ +import { captureException, withScope } from '@sentry/core'; +import { addExceptionMechanism, isNodeEnv, isString } from '@sentry/utils'; + +import type { ErrorResponse } from '../utils/types'; + +/** + * Checks whether the given error is an ErrorResponse. + * ErrorResponse is when users throw a response from their loader or action functions. + * This is in fact a server-side error that we capture on the client. + * + * @param error The error to check. + * @returns boolean + */ +function isErrorResponse(error: unknown): error is ErrorResponse { + return typeof error === 'object' && error !== null && 'status' in error && 'statusText' in error; +} + +/** + * Captures an error that is thrown inside a Remix ErrorBoundary. + * + * @param error The error to capture. + * @returns void + */ +export function captureRemixErrorBoundaryError(error: unknown): void { + const isClientSideRuntimeError = !isNodeEnv() && error instanceof Error; + const isRemixErrorResponse = isErrorResponse(error); + // Server-side errors apart from `ErrorResponse`s also appear here without their stacktraces. + // So, we only capture: + // 1. `ErrorResponse`s + // 2. Client-side runtime errors here, + // And other server - side errors in `handleError` function where stacktraces are available. + if (isRemixErrorResponse || isClientSideRuntimeError) { + const eventData = isRemixErrorResponse + ? { + function: 'ErrorResponse', + ...error.data, + } + : { + function: 'ReactError', + }; + + withScope(scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'instrument', + handled: true, + data: eventData, + }); + return event; + }); + + if (isRemixErrorResponse) { + if (isString(error.data)) { + captureException(error.data); + } else if (error.statusText) { + captureException(error.statusText); + } else { + captureException(error); + } + } else { + captureException(error); + } + }); + } +} diff --git a/packages/remix/src/performance/client.tsx b/packages/remix/src/client/performance.tsx similarity index 95% rename from packages/remix/src/performance/client.tsx rename to packages/remix/src/client/performance.tsx index 879c93e51f42..a3f7815b7bdc 100644 --- a/packages/remix/src/performance/client.tsx +++ b/packages/remix/src/client/performance.tsx @@ -4,6 +4,8 @@ import type { Transaction, TransactionContext } from '@sentry/types'; import { isNodeEnv, logger } from '@sentry/utils'; import * as React from 'react'; +import { getFutureFlagsBrowser } from '../utils/futureFlags'; + const DEFAULT_TAGS = { 'routing.instrumentation': 'remix-router', } as const; @@ -93,7 +95,8 @@ export function withSentry

, R extends React.FC wrapWithErrorBoundary?: boolean; errorBoundaryOptions?: ErrorBoundaryProps; } = { - wrapWithErrorBoundary: true, + // We don't want to wrap application with Sentry's ErrorBoundary by default for Remix v2 + wrapWithErrorBoundary: getFutureFlagsBrowser()?.v2_errorBoundary ? false : true, errorBoundaryOptions: {}, }, ): R { diff --git a/packages/remix/src/index.client.tsx b/packages/remix/src/index.client.tsx index 5c76ee4907bf..64951a3f10cd 100644 --- a/packages/remix/src/index.client.tsx +++ b/packages/remix/src/index.client.tsx @@ -3,7 +3,8 @@ import { configureScope, init as reactInit } from '@sentry/react'; import { buildMetadata } from './utils/metadata'; import type { RemixOptions } from './utils/remixOptions'; -export { remixRouterInstrumentation, withSentry } from './performance/client'; +export { remixRouterInstrumentation, withSentry } from './client/performance'; +export { captureRemixErrorBoundaryError } from './client/errors'; export * from '@sentry/react'; export function init(options: RemixOptions): void { diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index 21cfc20b17ab..9b8ad52e18f0 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -6,8 +6,10 @@ import { instrumentServer } from './utils/instrumentServer'; import { buildMetadata } from './utils/metadata'; import type { RemixOptions } from './utils/remixOptions'; +export { captureRemixServerException } from './utils/instrumentServer'; export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; -export { remixRouterInstrumentation, withSentry } from './performance/client'; +export { remixRouterInstrumentation, withSentry } from './client/performance'; +export { captureRemixErrorBoundaryError } from './client/errors'; export * from '@sentry/node'; export { wrapExpressCreateRequestHandler } from './utils/serverAdapters/express'; diff --git a/packages/remix/src/utils/futureFlags.ts b/packages/remix/src/utils/futureFlags.ts new file mode 100644 index 000000000000..b9200d100a0e --- /dev/null +++ b/packages/remix/src/utils/futureFlags.ts @@ -0,0 +1,35 @@ +import { GLOBAL_OBJ } from '@sentry/utils'; + +import type { FutureConfig, ServerBuild } from './types'; + +export type EnhancedGlobal = typeof GLOBAL_OBJ & { + __remixContext?: { + future?: FutureConfig; + }; +}; + +/** + * Get the future flags from the Remix browser context + * + * @returns The future flags + */ +export function getFutureFlagsBrowser(): FutureConfig | undefined { + const window = GLOBAL_OBJ as EnhancedGlobal; + + if (!window.__remixContext) { + return; + } + + return window.__remixContext.future; +} + +/** + * Get the future flags from the Remix server build + * + * @param build The Remix server build + * + * @returns The future flags + */ +export function getFutureFlagsServer(build: ServerBuild): FutureConfig | undefined { + return build.future; +} diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 5e0f300bf82e..2087a892d1f7 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -14,12 +14,14 @@ import { logger, } from '@sentry/utils'; +import { getFutureFlagsServer } from './futureFlags'; import type { AppData, CreateRequestHandlerFunction, DataFunction, DataFunctionArgs, EntryContext, + FutureConfig, HandleDocumentRequestFunction, ReactRouterDomPkg, RemixRequest, @@ -31,6 +33,8 @@ import type { import { extractData, getRequestMatch, isDeferredData, isResponse, json, matchServerRoutes } from './vendor/response'; import { normalizeRemixRequest } from './web-fetch'; +let FUTURE_FLAGS: FutureConfig | undefined; + // Flag to track if the core request handler is instrumented. export let isRequestHandlerWrapped = false; @@ -57,7 +61,16 @@ async function extractResponseError(response: Response): Promise { return responseData; } -async function captureRemixServerException(err: unknown, name: string, request: Request): Promise { +/** + * Captures an exception happened in the Remix server. + * + * @param err The error to capture. + * @param name The name of the origin function. + * @param request The request object. + * + * @returns A promise that resolves when the exception is captured. + */ +export async function captureRemixServerException(err: unknown, name: string, request: Request): Promise { // Skip capturing if the thrown error is not a 5xx response // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders if (isResponse(err) && err.status < 500) { @@ -146,7 +159,10 @@ function makeWrappedDocumentRequestFunction( span?.finish(); } catch (err) { - await captureRemixServerException(err, 'documentRequest', request); + if (!FUTURE_FLAGS?.v2_errorBoundary) { + await captureRemixServerException(err, 'documentRequest', request); + } + throw err; } @@ -183,7 +199,10 @@ function makeWrappedDataFunction(origFn: DataFunction, id: string, name: 'action currentScope.setSpan(activeTransaction); span?.finish(); } catch (err) { - await captureRemixServerException(err, name, args.request); + if (!FUTURE_FLAGS?.v2_errorBoundary) { + await captureRemixServerException(err, name, args.request); + } + throw err; } @@ -430,6 +449,7 @@ function makeWrappedCreateRequestHandler( isRequestHandlerWrapped = true; return function (this: unknown, build: ServerBuild, ...args: unknown[]): RequestHandler { + FUTURE_FLAGS = getFutureFlagsServer(build); const newBuild = instrumentBuild(build); const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); diff --git a/packages/remix/src/utils/types.ts b/packages/remix/src/utils/types.ts index 74dcf10215cc..faaa7e5f6f60 100644 --- a/packages/remix/src/utils/types.ts +++ b/packages/remix/src/utils/types.ts @@ -14,6 +14,42 @@ import type * as Express from 'express'; import type { Agent } from 'https'; import type { ComponentType } from 'react'; +type Dev = { + command?: string; + scheme?: string; + host?: string; + port?: number; + restart?: boolean; + tlsKey?: string; + tlsCert?: string; +}; + +export interface FutureConfig { + unstable_dev: boolean | Dev; + /** @deprecated Use the `postcss` config option instead */ + unstable_postcss: boolean; + /** @deprecated Use the `tailwind` config option instead */ + unstable_tailwind: boolean; + v2_errorBoundary: boolean; + v2_headers: boolean; + v2_meta: boolean; + v2_normalizeFormMethod: boolean; + v2_routeConvention: boolean; +} + +export interface RemixConfig { + [key: string]: any; + future: FutureConfig; +} + +export interface ErrorResponse { + status: number; + statusText: string; + data: any; + error?: Error; + internal: boolean; +} + export type RemixRequestState = { method: string; redirect: RequestRedirect; @@ -133,6 +169,7 @@ export interface ServerBuild { assets: AssetsManifest; publicPath?: string; assetsBuildDirectory?: string; + future?: FutureConfig; } export interface HandleDocumentRequestFunction { diff --git a/packages/remix/test/integration/app_v2/entry.server.tsx b/packages/remix/test/integration/app_v2/entry.server.tsx index ae879492e236..c8e09b8129c1 100644 --- a/packages/remix/test/integration/app_v2/entry.server.tsx +++ b/packages/remix/test/integration/app_v2/entry.server.tsx @@ -1,4 +1,4 @@ -import type { EntryContext } from '@remix-run/node'; +import type { EntryContext, DataFunctionArgs } from '@remix-run/node'; import { RemixServer } from '@remix-run/react'; import { renderToString } from 'react-dom/server'; import * as Sentry from '@sentry/remix'; @@ -10,6 +10,14 @@ Sentry.init({ autoSessionTracking: false, }); +export function handleError(error: unknown, { request }: DataFunctionArgs): void { + if (error instanceof Error) { + Sentry.captureRemixServerException(error, 'remix.server', request); + } else { + Sentry.captureException(error); + } +} + export default function handleRequest( request: Request, responseStatusCode: number, diff --git a/packages/remix/test/integration/app_v2/root.tsx b/packages/remix/test/integration/app_v2/root.tsx index faf075951d69..3d84da307ad7 100644 --- a/packages/remix/test/integration/app_v2/root.tsx +++ b/packages/remix/test/integration/app_v2/root.tsx @@ -1,6 +1,15 @@ import { V2_MetaFunction, LoaderFunction, json, defer, redirect } from '@remix-run/node'; -import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; -import { withSentry } from '@sentry/remix'; +import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration, useRouteError } from '@remix-run/react'; +import { V2_ErrorBoundaryComponent } from '@remix-run/react/dist/routeModules'; +import { captureRemixErrorBoundaryError, withSentry } from '@sentry/remix'; + +export const ErrorBoundary: V2_ErrorBoundaryComponent = () => { + const error = useRouteError(); + + captureRemixErrorBoundaryError(error); + + return

error
; +}; export const meta: V2_MetaFunction = ({ data }) => [ { charset: 'utf-8' }, diff --git a/packages/remix/test/integration/common/routes/action-json-response.$id.tsx b/packages/remix/test/integration/common/routes/action-json-response.$id.tsx index 2c7a19059596..ff0f6940fe44 100644 --- a/packages/remix/test/integration/common/routes/action-json-response.$id.tsx +++ b/packages/remix/test/integration/common/routes/action-json-response.$id.tsx @@ -3,8 +3,10 @@ import { useActionData } from '@remix-run/react'; export const loader: LoaderFunction = async ({ params: { id } }) => { if (id === '-1') { - throw new Error('Unexpected Server Error from Loader'); + throw new Error('Unexpected Server Error'); } + + return null; }; export const action: ActionFunction = async ({ params: { id } }) => { diff --git a/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx b/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx index 55b53e2d70dc..a4ad3dc48339 100644 --- a/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx +++ b/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx @@ -5,7 +5,7 @@ type LoaderData = { id: string }; export const loader: LoaderFunction = async ({ params: { id } }) => { if (id === '-2') { - throw new Error('Unexpected Server Error from Loader'); + throw new Error('Unexpected Server Error'); } if (id === '-1') { diff --git a/packages/remix/test/integration/test/client/errorboundary.test.ts b/packages/remix/test/integration/test/client/errorboundary.test.ts index b90b3e8d3eaa..9c496e3e3040 100644 --- a/packages/remix/test/integration/test/client/errorboundary.test.ts +++ b/packages/remix/test/integration/test/client/errorboundary.test.ts @@ -21,16 +21,20 @@ test('should capture React component errors.', async ({ page }) => { expect(errorEnvelope.level).toBe('error'); expect(errorEnvelope.sdk?.name).toBe('sentry.javascript.remix'); expect(errorEnvelope.exception?.values).toMatchObject([ - { - type: 'React ErrorBoundary Error', - value: 'Sentry React Component Error', - stacktrace: { frames: expect.any(Array) }, - }, + ...(!useV2 + ? [ + { + type: 'React ErrorBoundary Error', + value: 'Sentry React Component Error', + stacktrace: { frames: expect.any(Array) }, + }, + ] + : []), { type: 'Error', value: 'Sentry React Component Error', stacktrace: { frames: expect.any(Array) }, - mechanism: { type: 'generic', handled: true }, + mechanism: { type: useV2 ? 'instrument' : 'generic', handled: true }, }, ]); }); diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index cc25a87611d4..a2a4632ba962 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -81,7 +81,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'remix.server' : 'action', }, handled: true, type: 'instrument', @@ -197,11 +197,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada values: [ { type: 'Error', - value: 'Unexpected Server Error from Loader', + value: 'Unexpected Server Error', stacktrace: expect.any(Object), mechanism: { data: { - function: 'loader', + function: useV2 ? 'remix.server' : 'loader', }, handled: true, type: 'instrument', @@ -254,7 +254,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', @@ -303,11 +303,13 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada values: [ { type: 'Error', - value: 'Non-Error exception captured with keys: data', + value: useV2 + ? 'Non-Error exception captured with keys: data, internal, status, statusText' + : 'Non-Error exception captured with keys: data', stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', @@ -360,7 +362,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', @@ -409,11 +411,13 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada values: [ { type: 'Error', - value: 'Non-Error exception captured with keys: [object has no keys]', + value: useV2 + ? 'Non-Error exception captured with keys: data, internal, status, statusText' + : 'Non-Error exception captured with keys: [object has no keys]', stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 8a99c699cc37..ccaa93b05e36 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -35,11 +35,11 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada values: [ { type: 'Error', - value: 'Unexpected Server Error from Loader', + value: 'Unexpected Server Error', stacktrace: expect.any(Object), mechanism: { data: { - function: 'loader', + function: useV2 ? 'remix.server' : 'loader', }, handled: true, type: 'instrument', @@ -134,11 +134,11 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada values: [ { type: 'Error', - value: 'Unexpected Server Error from Loader', + value: 'Unexpected Server Error', stacktrace: expect.any(Object), mechanism: { data: { - function: 'loader', + function: useV2 ? 'remix.server' : 'loader', }, handled: true, type: 'instrument',