feat(remix): Add Remix server SDK #5269
Conversation
size-limit report 📦
|
AbhiPrasad
left a comment
AbhiPrasad
left a comment
There was a problem hiding this comment.
I like how simple wrapping the handlers are!
|
|
||
| // debugger; | ||
| const transaction = hasTracingEnabled() | ||
| ? startTransaction({ |
There was a problem hiding this comment.
startTransaction should no-op if tracing is not enabled, so we don't need the ternary, we can just always call startTransaction
packages/remix/src/flags.ts
Outdated
| declare const __SENTRY_DEBUG__: boolean; | ||
|
|
||
| /** Flag that is true for debug builds, false otherwise. */ | ||
| export const IS_DEBUG_BUILD = typeof __SENTRY_DEBUG__ === 'undefined' ? true : __SENTRY_DEBUG__; |
There was a problem hiding this comment.
We can remove this file. Please see https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md#debug-build-flags for more details.
We should just be able to use __DEBUG_BUILD__ everywhere now.
packages/remix/src/index.server.ts
Outdated
| export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; | ||
| export { remixRouterInstrumentation, withSentryRouteTracing } from './performance/client'; | ||
| export { BrowserTracing, Integrations } from '@sentry/tracing'; | ||
| export * from '@sentry/node'; |
There was a problem hiding this comment.
let's move these exports to the top
| type AppLoadContext = unknown; | ||
| type AppData = unknown; | ||
| type RequestHandler = (request: Request, loadContext?: AppLoadContext) => Promise<Response>; | ||
| type CreateRequestHandlerFunction = (build: ServerBuild, mode?: string) => RequestHandler; |
There was a problem hiding this comment.
Are these all vendored in? Can we link to the code + git sha where we did this?
| let res: Response | AppData; | ||
| const activeTransaction = getActiveTransaction(); | ||
| const currentScope = getCurrentHub().getScope(); | ||
|
|
There was a problem hiding this comment.
Instead of using optional chaining, let's just early return if there is no activeTransaction. Same if there is no currentScope
| } | ||
|
|
||
| res = await origFn.call(this, args); | ||
| span?.finish(); |
There was a problem hiding this comment.
Should we re-add back the transaction onto the scope?
currentScope.setSpan(activeTransaction);
span.finish();There was a problem hiding this comment.
Why is optional chaining still needed here?
| return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> { | ||
| const currentScope = getCurrentHub().getScope(); | ||
|
|
||
| // debugger; |
There was a problem hiding this comment.
| // debugger; |
| }, | ||
| "sideEffects": [ | ||
| "./esm/index.server.js", | ||
| "./src/index.server.ts" |
There was a problem hiding this comment.
Why do these have side effects?
There was a problem hiding this comment.
I took that from Next.JS SDK.
As I understand, we need to export browser / react stuff from index.server.ts to make TypeScript happy on compile time, but it's not guaranteed that it'll be used on runtime. (index.client.ts is used instead on the demo project).
So my guess is that flagging index.server.* as side effects is to prevent a pre-bundler that may fail TS compilation.
| } | ||
|
|
||
| res = await origFn.call(this, args); | ||
| span?.finish(); |
There was a problem hiding this comment.
Why is optional chaining still needed here?
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Part 3 of #4894
Adds server side SDK for error tracking / performance tracing of Remix.
createRequestHandlerfrom@remix-run/server-runtimewhich apparently is used by all server-side adapters of Remix.actionandloaderfunctions are patched as parameters ofcreateRequestHandler.Tested:
action/loaderrequestHandlerLink to Transaction
Link to Event