Fix(integrations): Fix support for wildcard paths in react-router-dom v6 integration (#5997) #14076
Conversation
There was a problem hiding this comment.
Thanks for this @grahamhency! Just had a first quick pass, and it seems this fixes the wildcard use in some cases. Though, it does not fix the use of Descendant Routes (#5997). We can try to address that separately.
| @@ -250,7 +250,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { | |||
| <MemoryRouter initialEntries={['/']}> | |||
| <SentryRoutes> | |||
| <Route path="/about" element={<div>About</div>}> | |||
| <Route path="/about/us" element={<div>us</div>} /> | |||
There was a problem hiding this comment.
We still need to cover this usage in tests to be sure we're not breaking anything. This is still a valid use. Could you please instead add a new test case as you update?
| @@ -141,6 +141,14 @@ function stripBasenameFromPathname(pathname: string, basename: string): string { | |||
| return pathname.slice(startIndex) || '/'; | |||
| } | |||
|
|
|||
| function sendIndexPath(pathBuilder: string, pathname: string, basename: string): void | string[] { | |||
| const p1 = pathBuilder || stripBasenameFromPathname(pathname, basename); | |||
There was a problem hiding this comment.
This is a little hard to track. Could you please add some comments here?
|
|
||
| expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); | ||
| expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { | ||
| name: '/projects/:projectId/views/:viewId', |
There was a problem hiding this comment.
Can we also add a test case where the wildcard exists in the transaction name (like: /param-page/*/details)? Something similar to the example app you provided.
Based on @grahamhency's work at #14076. Sets correct transaction names when wildcards are used in React Router 6 routes. This does not fix the usage of the descendant routes (#5997), which we should try to address separately.
|
Closing this because we merged in #14205 - thanks for the PR @grahamhency! |
|
@AbhiPrasad ah, thank you! Sorry I couldn't see this PR through, my work work life got in the way hah. Glad I could help! |
|
You got the ball rolling! That mattered. I created a PR to give you acknowledgement in our changelog as well: #14228 - that will be published with the next release. |
This PR attempts to address the issue described in #5997 where wildcard paths (
*) in the react-router-dom v6 integration were not being properly reported on for transactions.To test:
I also added a test for this change. I noticed that the existing tests had odd react router route declarations as far as nesting goes so I updated those tests to reflect how nested routes within react router should be structured.