fix(react): Fix React Router v6 paramaterization #5515
Conversation
Make sure paramaterization occurs in scenarios where the full path is not avaliable after all the branches are walked. In those scenarios, we have to construct the paramaterized name based on the previous branches that were analyzed.
AbhiPrasad
requested review from
onurtemizkan,
a team,
lforst and
Lms24
and removed request for
a team
| // eslint-disable-next-line @typescript-eslint/prefer-for-of | ||
| for (let x = 0; x < branches.length; x++) { |
There was a problem hiding this comment.
Just out of curiosity: why the index for loop?
There was a problem hiding this comment.
It's to stay consistent with RR5 integration. Not sure if there's any specific reason there though.
sentry-javascript/packages/react/src/reactrouter.tsx
Lines 79 to 84 in 9b7131f
There was a problem hiding this comment.
I'll just keep the explicit for loop, though technically it is extra bytes
packages/react/src/reactrouterv6.tsx
Outdated
| // If the route defined on the element is something like | ||
| // <Route path="/stores/:storeId/products/:productId" element={<div>Product</div>} /> | ||
| // We should check against the branch.pathname for the number of / seperators | ||
| if (pathBuilder.split('/').length !== branch.pathname.split('/').length) { |
There was a problem hiding this comment.
Is this a safe check? Just asking because I used URL segment counting in my Express router instrumentation and I had to handle a few edge cases. For example, could there be problems with routes starting or ending with a / while others don't? (which would influence the length because of empty array items after the .split operation).
Just a suggestion but now that I'm thinking about it, we could extract this function
sentry-javascript/packages/tracing/src/integrations/node/express.ts
Lines 387 to 394 in 6ea53ed
to our utils package and use it here. But totally optional ofc.
There was a problem hiding this comment.
Great point! I'm going to extract out getNumberOfUrlSegments and use it instead.
Previous versions did offer regex/array support - this one no longer does, so I think we are fine here. Edit: https://reactrouter.com/docs/en/v6/getting-started/faq#what-happened-to-regexp-routes-paths |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return {}; | ||
| } | ||
|
|
||
| const match = url.match(/^(([^:/?#]+):)?(\/\/([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?$/); |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data
size-limit report 📦
|
Fixes #5513
Make sure paramaterization occurs in scenarios where the full path is not available after all the branches are walked. In those scenarios, we have to construct the paramaterized name based on the previous branches that were analyzed.
This patch also creates a new file in
@sentry/utils-url.ts, where we store some url / string related utils. This was made so that the react package would get access to thegetNumberOfUrlSegmentsutil.