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): Use Set as the allRoutes container. #14878

Merged
merged 3 commits into from
Jan 2, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from 'react';
import ReactDOM from 'react-dom/client';
import {
BrowserRouter,
Outlet,
Route,
Routes,
createRoutesFromChildren,
Expand Down Expand Up @@ -48,17 +49,28 @@ const DetailsRoutes = () => (
</SentryRoutes>
);

const DetailsRoutesAlternative = () => (
<SentryRoutes>
<Route path=":detailId" element={<div id="details">Details</div>} />
</SentryRoutes>
);

const ViewsRoutes = () => (
<SentryRoutes>
<Route index element={<div id="views">Views</div>} />
<Route path="views/:viewId/*" element={<DetailsRoutes />} />
<Route path="old-views/:viewId/*" element={<DetailsRoutesAlternative />} />
</SentryRoutes>
);

const ProjectsRoutes = () => (
<SentryRoutes>
<Route path="projects/:projectId/*" element={<ViewsRoutes />}></Route>
<Route path="*" element={<div>No Match Page</div>} />
<Route path="projects" element={<Outlet />}>
<Route index element={<div>Project Page Root</div>} />
<Route path="*" element={<Outlet />}>
<Route path=":projectId/*" element={<ViewsRoutes />} />
</Route>
</Route>
</SentryRoutes>
);

Expand All @@ -67,7 +79,7 @@ root.render(
<BrowserRouter>
<SentryRoutes>
<Route path="/" element={<Index />} />
<Route path="/*" element={<ProjectsRoutes />}></Route>
<Route path="/*" element={<ProjectsRoutes />} />
</SentryRoutes>
</BrowserRouter>,
);
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ const Index = () => {
<Link to="/projects/123/views/456/789" id="navigation">
navigate
</Link>
<Link to="/projects/123/old-views/345/654" id="old-navigation">
navigate old
</Link>
</>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) =

const rootSpan = await transactionPromise;

expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
expect(rootSpan).toMatchObject({
contexts: {
trace: {
Expand All @@ -24,6 +25,30 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) =
});
});

test('sends a pageload transaction with a parameterized URL - alternative route', async ({ page }) => {
const transactionPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
});

await page.goto(`/projects/234/old-views/234/567`);

const rootSpan = await transactionPromise;

expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
expect(rootSpan).toMatchObject({
contexts: {
trace: {
op: 'pageload',
origin: 'auto.pageload.react.reactrouter_v6',
},
},
transaction: '/projects/:projectId/old-views/:viewId/:detailId',
transaction_info: {
source: 'route',
},
});
});

test('sends a navigation transaction with a parameterized URL', async ({ page }) => {
const pageloadTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
Expand Down Expand Up @@ -52,6 +77,8 @@ test('sends a navigation transaction with a parameterized URL', async ({ page })
const linkElement = page.locator('id=navigation');

const [_, navigationTxn] = await Promise.all([linkElement.click(), navigationTxnPromise]);

expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
expect(navigationTxn).toMatchObject({
contexts: {
trace: {
Expand All @@ -65,3 +92,47 @@ test('sends a navigation transaction with a parameterized URL', async ({ page })
},
});
});

test('sends a navigation transaction with a parameterized URL - alternative route', async ({ page }) => {
const pageloadTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
});

const navigationTxnPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'navigation';
});

await page.goto(`/`);
const pageloadTxn = await pageloadTxnPromise;

expect(pageloadTxn).toMatchObject({
contexts: {
trace: {
op: 'pageload',
origin: 'auto.pageload.react.reactrouter_v6',
},
},
transaction: '/',
transaction_info: {
source: 'route',
},
});

const linkElement = page.locator('id=old-navigation');

const [_, navigationTxn] = await Promise.all([linkElement.click(), navigationTxnPromise]);

expect((await page.innerHTML('#root')).includes('Details')).toBe(true);
expect(navigationTxn).toMatchObject({
contexts: {
trace: {
op: 'navigation',
origin: 'auto.navigation.react.reactrouter_v6',
},
},
transaction: '/projects/:projectId/old-views/:viewId/:detailId',
transaction_info: {
source: 'route',
},
});
});
49 changes: 34 additions & 15 deletions packages/react/src/reactrouterv6-compat-utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio
return origUseRoutes;
}

const allRoutes: RouteObject[] = [];
const allRoutes: Set<RouteObject> = new Set();

const SentryRoutes: React.FC<{
children?: React.ReactNode;
Expand All @@ -206,18 +206,29 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio

if (isMountRenderPass.current) {
routes.forEach(route => {
allRoutes.push(...getChildRoutesRecursively(route));
const extractedChildRoutes = getChildRoutesRecursively(route);

extractedChildRoutes.forEach(r => {
allRoutes.add(r);
});
});

updatePageloadTransaction(getActiveRootSpan(), normalizedLocation, routes, undefined, undefined, allRoutes);
updatePageloadTransaction(
getActiveRootSpan(),
normalizedLocation,
routes,
undefined,
undefined,
Array.from(allRoutes),
);
isMountRenderPass.current = false;
} else {
handleNavigation({
location: normalizedLocation,
routes,
navigationType,
version,
allRoutes,
allRoutes: Array.from(allRoutes),
});
}
}, [navigationType, stableLocationParam]);
Expand Down Expand Up @@ -342,14 +353,18 @@ function locationIsInsideDescendantRoute(location: Location, routes: RouteObject
return false;
}

function getChildRoutesRecursively(route: RouteObject, allRoutes: RouteObject[] = []): RouteObject[] {
if (route.children && !route.index) {
route.children.forEach(child => {
allRoutes.push(...getChildRoutesRecursively(child, allRoutes));
});
}
function getChildRoutesRecursively(route: RouteObject, allRoutes: Set<RouteObject> = new Set()): Set<RouteObject> {
if (!allRoutes.has(route)) {
allRoutes.add(route);

allRoutes.push(route);
if (route.children && !route.index) {
route.children.forEach(child => {
const childRoutes = getChildRoutesRecursively(child, allRoutes);

childRoutes.forEach(r => allRoutes.add(r));
});
}
}

return allRoutes;
}
Expand Down Expand Up @@ -510,7 +525,7 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<
return Routes;
}

const allRoutes: RouteObject[] = [];
const allRoutes: Set<RouteObject> = new Set();

const SentryRoutes: React.FC<P> = (props: P) => {
const isMountRenderPass = React.useRef(true);
Expand All @@ -524,18 +539,22 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<

if (isMountRenderPass.current) {
routes.forEach(route => {
allRoutes.push(...getChildRoutesRecursively(route));
const extractedChildRoutes = getChildRoutesRecursively(route);

extractedChildRoutes.forEach(r => {
allRoutes.add(r);
});
});

updatePageloadTransaction(getActiveRootSpan(), location, routes, undefined, undefined, allRoutes);
updatePageloadTransaction(getActiveRootSpan(), location, routes, undefined, undefined, Array.from(allRoutes));
isMountRenderPass.current = false;
} else {
handleNavigation({
location,
routes,
navigationType,
version,
allRoutes,
allRoutes: Array.from(allRoutes),
});
}
},
Expand Down
Loading
Loading