- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.7k
          feat(react): Add reactRouterV4/V5BrowserTracingIntegration for react router v4 & v5
          #10488
        
          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
Conversation
This adds new `browserTracingReactRouterV4Integration()` and `browserTracingReactRouterV5Integration()` 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.
| size-limit report 📦
 | 
browserTracingIntegration for react router v4 & v5
      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.
Lgtm! Cleaning this stuff up is gonna be hella satisfying.
        
          
                packages/react/src/index.ts
              
                Outdated
          
        
      | browserTracingReactRouterV4Integration, | ||
| browserTracingReactRouterV5Integration, | 
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.
Idk if this makes sense but reactRouterV5BrowserTracingIntegration would sound better to me. Any reason you chose this order? ^^
…owserTracingIntegration`
browserTracingIntegration for react router v4 & v5reactRouterV4/V5BrowserTracingIntegration for react router v4 & v5
      | } | ||
| /* eslint-enable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access */ | ||
|  | ||
| function getActiveRootSpan(): Span | undefined { | 
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.
might be worth lifting this utility out
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.
yeah, but here it's a bit special because it also takes the activeTransaction into consideration!
This adds new
reactRouterV4BrowserTracingIntegration()andreactRouterV5BrowserTracingIntegration()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