Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(remix): Wrap root with ErrorBoundary. #5365

Merged
merged 6 commits into from Jul 8, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Jul 5, 2022

Ref: #4894

Wraps the Remix root with ErrorBoundary while wrapping it with the router instrumentation.

withSentryRouteTracing can be renamed to something like withSentry from this point, but it is a breaking change, so I left it out.

@@ -135,5 +137,5 @@ export function withSentryRouteTracing<P extends Record<string, unknown>, R exte

// @ts-ignore Setting more specific React Component typing for `R` generic above
// will break advanced type inference done by react router params
return SentryRoot;
return withErrorBoundary(SentryRoot);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’ll need to make sure users can supply errorboundary options, so things like fallback can be configured. We can pass it in as an option?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note is what we do if someone doesn’t want an error boundary here.

@@ -71,24 +71,22 @@ import {
ScrollRestoration,
} from "@remix-run/react";

import { ErrorBoundary, withSentryRouteTracing } from "@sentry/remix";
import { withSentryRouteTracing } from "@sentry/remix";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep withSentryRouteTracing export, deprecate it, and then export withSentry

@AbhiPrasad AbhiPrasad mentioned this pull request Jul 5, 2022
26 tasks
@AbhiPrasad AbhiPrasad added this to the Sentry Remix SDK milestone Jul 5, 2022
@onurtemizkan
Copy link
Collaborator Author

@AbhiPrasad, updated with the options and a toggle.

It requires #5371 to work.

@onurtemizkan onurtemizkan self-assigned this Jul 6, 2022
);
}

export default withSentryRouteTracing(App);
export default withSentryRouteTracing(App, {
wrapWithErrorBoundary: false // default: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wrapWithErrorBoundary: false // default: true

We shouldn't have the docs saying that it's disabled. We can just leave a note that they can use the wrapWithErrorBoundary to disable if needed.

@onurtemizkan onurtemizkan force-pushed the onur/remix-router-with-error-boundary branch from 3e05280 to 5704702 Compare July 6, 2022 15:10
@AbhiPrasad
Copy link
Member

Let's rename to withSentry? We can alias to withSentryRouteTracing for backwards compatibility, but just mark it @deprecated

@onurtemizkan onurtemizkan force-pushed the onur/remix-router-with-error-boundary branch from 341cd65 to 0a59239 Compare July 6, 2022 16:42
@AbhiPrasad AbhiPrasad enabled auto-merge (squash) July 6, 2022 16:57
@AbhiPrasad AbhiPrasad disabled auto-merge July 6, 2022 16:58
@vladanpaunovic
Copy link
Contributor

@onurtemizkan, as we are moving to withSentry, let's completely remove withSentryRouteTracing.

We don't have to worry about backwards compatibility here yet. We are in alpha versions and breaking changes are expected.

@AbhiPrasad AbhiPrasad merged commit ae086f9 into master Jul 8, 2022
@AbhiPrasad AbhiPrasad deleted the onur/remix-router-with-error-boundary branch July 8, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants