-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(react): improve handling of routes nested under path="/" (#14821)
We noticed this in Sentry's `/issues/:groupId/` route, which uses SDK v8.43.0. The full route tree is complex, but the relevant parts are: ```js <Route path="/" element={<div>root</div>}> <Route path="/issues/:groupId/" element={<div>issues group</div>}> <Route index element={<div>index</div>} /> </Route> </Route> ``` If you navigate to e.g. `/issues/123` (no trailing slash), the recorded transaction name is `//issues/:groupId/` (notice the double slash). This looks messy but doesn't have too much of a consequence. The worse issue is when you navigate to e.g. `/issues/123/` (with trailing slash), the transaction name is `/issues/123/` - it is not parameterized. This breaks transaction grouping. On the `master` and `develop` branch of the SDK, the transaction name is recorded as `/`. This causes the transactions to be grouped incorrectly with the root, as well as any other routes nested under a route with `path="/"`. (Thanks @JoshFerge for noticing the bad data in Sentry! 🙌) --- Note that this commit effectively reverts a change from #14304 where ```js if (basename + branch.pathname === location.pathname) { ``` became ```js if (location.pathname.endsWith(basename + branch.pathname)) { ``` This is the change that caused the difference in results between SDK versions. This will always match when `basename` is empty, `branch.pathname` is `/`, and `location.pathname` ends with `/` - in other words, if you have a parent route with `path="/"`, every request ending in a slash will match it instead of the more specific child route. (Depending on how often this kind of `Route` nesting happens in the wild, this could be a wide regression in transaction names.) But, no tests failed from the removal of `endsWith`. @onurtemizkan, who wrote the change in question: > I'd expect this to break descendant routes. But in the final > implementation, descendant routes don't end up here. So, I > believe it's safe to revert.
- Loading branch information
Showing
2 changed files
with
158 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters