Skip to content
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

fix(react): Fix React Router v6 paramaterization #5515

Merged
merged 4 commits into from Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 35 additions & 3 deletions packages/react/src/reactrouterv6.tsx
Expand Up @@ -20,9 +20,23 @@ type Params<Key extends string = string> = {
readonly [key in Key]: string | undefined;
};

// https://github.com/remix-run/react-router/blob/9fa54d643134cd75a0335581a75db8100ed42828/packages/react-router/lib/router.ts#L114-L134
interface RouteMatch<ParamKey extends string = string> {
/**
* The names and values of dynamic parameters in the URL.
*/
params: Params<ParamKey>;
/**
* The portion of the URL pathname that was matched.
*/
pathname: string;
/**
* The portion of the URL pathname that was matched before child routes.
*/
pathnameBase: string;
/**
* The route object that was used to match.
*/
route: RouteObject;
}

Expand Down Expand Up @@ -94,13 +108,31 @@ function getNormalizedName(

const branches = matchRoutes(routes, location);

let pathBuilder = '';
if (branches) {
// eslint-disable-next-line @typescript-eslint/prefer-for-of
for (let x = 0; x < branches.length; x++) {
Comment on lines 113 to 114
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity: why the index for loop?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to stay consistent with RR5 integration. Not sure if there's any specific reason there though.

// eslint-disable-next-line @typescript-eslint/prefer-for-of
for (let x = 0; x < branches.length; x++) {
if (branches[x].match.isExact) {
return [branches[x].match.path, 'route'];
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just keep the explicit for loop, though technically it is extra bytes

if (branches[x].route && branches[x].route.path && branches[x].pathname === location.pathname) {
const path = branches[x].route.path;
const branch = branches[x];
const route = branch.route;
if (route) {
// Early return if index route
if (route.index) {
return [branch.pathname, 'route'];
}

const path = route.path;
if (path) {
return [path, 'route'];
const newPath = path[0] === '/' ? path : `/${path}`;
pathBuilder += newPath;
if (branch.pathname === location.pathname) {
// 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) {
Copy link
Member

@Lms24 Lms24 Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Returns number of URL segments of a passed URL.
* Also handles URLs of type RegExp
*/
function getNumberOfUrlSegments(url: string): number {
// split at '/' or at '\/' to split regex urls correctly
return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length;
}

to our utils package and use it here. But totally optional ofc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! I'm going to extract out getNumberOfUrlSegments and use it instead.

return [newPath, 'route'];
}
return [pathBuilder, 'route'];
}
}
}
}
Expand Down
34 changes: 34 additions & 0 deletions packages/react/test/reactrouterv6.test.tsx
Expand Up @@ -196,4 +196,38 @@ describe('React Router v6', () => {
metadata: { source: 'route' },
});
});

it('works with nested paths with parameters', () => {
const [mockStartTransaction] = createInstrumentation();
const SentryRoutes = withSentryReactRouterV6Routing(Routes);

render(
<MemoryRouter initialEntries={['/']}>
<SentryRoutes>
<Route index element={<Navigate to="/projects/123/views/234" />} />
<Route path="account" element={<div>Account Page</div>} />
<Route path="projects">
<Route index element={<div>Project Index</div>} />
<Route path=":projectId" element={<div>Project Page</div>}>
<Route index element={<div>Project Page Root</div>} />
<Route element={<div>Editor</div>}>
<Route path="views/:viewId" element={<div>View Canvas</div>} />
<Route path="spaces/:spaceId" element={<div>Space Canvas</div>} />
</Route>
</Route>
</Route>

<Route path="*" element={<div>No Match Page</div>} />
</SentryRoutes>
</MemoryRouter>,
);

expect(mockStartTransaction).toHaveBeenCalledTimes(2);
expect(mockStartTransaction).toHaveBeenLastCalledWith({
name: '/projects/:projectId/views/:viewId',
op: 'navigation',
tags: { 'routing.instrumentation': 'react-router-v6' },
metadata: { source: 'route' },
});
});
});