-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(react): React Router v4/v5 integration #10430
Conversation
// TODO (v8): Remove this check and just return span | ||
// eslint-disable-next-line deprecation/deprecation | ||
return span.transaction; | ||
return span && span.transaction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed this because the integration should only be changing the name of the root span (for transaction name paramaterization).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm not sure, is it not cleaner to do span ? getRootSpan(span) : undefined
, or do we need this so often? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means I can do getRootSpan(getActiveSpan())
, which helps with DX a lot
|
||
export const V4_SETUP_CLIENTS = new WeakMap<Client, boolean>(); | ||
|
||
export const V5_SETUP_CLIENTS = new WeakMap<Client, boolean>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These weak maps are used to validate if the integration should work with withSentryRouting
helper.
/** | ||
* Options for React Router v4 and v4 integration | ||
*/ | ||
type ReactRouterOptions = DefaultReactRouterOptions | RouteConfigReactRouterOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This union makes the DX waaay nicer, but I don't want to port it to the routingInstrumentation
in fear of breaking anything.
size-limit report 📦
|
type ReactRouterOptions = DefaultReactRouterOptions | RouteConfigReactRouterOptions; | ||
|
||
// @ts-expect-error Don't type narrow on routes or matchPath to save on bundle size | ||
const _reactRouterV4 = (({ history, routes, matchPath }: ReactRouterOptions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the main thing I see here is - or maybe I am missing this somehow - that we do not disable the default page load/navigation spans emitted by the default browserTracingIntegration()
here 🤔 So this would be emitted twice when a router integration is added, wouldn't it?
Upon further conversations async we decided to make this an integration that wraps |
…t router v4 & v5 (#10488) This adds new `reactRouterV4BrowserTracingIntegration()` and `reactRouterV5BrowserTracingIntegration()` exports, deprecating these old routing instrumentations. I opted to leave as much as possible as-is for now, except for streamlining the attributes/tags we use for the instrumentation. Tests lifted from #10430
ref #10387
This PR adds two integrations for react router v4 and v5, meant to be used with the new
browserTracingIntegration
. The reason that we can't adjust the method forbrowserTracing
is because react router has multiple versions we support (and is optional because there are multiple react routing libraries), so we have to make it a separate integration.Setup is simple!
I also took the liberty to re-organize the files, everything should be validated by unit (actually they're basically integration) tests.
I didn't deprecate the
routingInstrumentation
yet, figured we should do that when we deprecateBrowserTracing
in general.