From 0324eb7f6b7218e330aa41c8c86d69866b941820 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 14 Nov 2024 13:38:50 +0000 Subject: [PATCH 01/13] fix(react): Add React Router Descendant Routes support. --- .../react/src/reactrouterv6-compat-utils.tsx | 154 +++++++++++++++--- packages/react/src/reactrouterv6.tsx | 4 + packages/react/test/reactrouterv6.4.test.tsx | 2 +- packages/react/test/reactrouterv6.test.tsx | 49 ++++++ 4 files changed, 188 insertions(+), 21 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 7752e49d69a1..6cdde5c1e940 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -1,3 +1,4 @@ +/* eslint-disable complexity */ /* eslint-disable max-lines */ // Inspired from Donnie McNeal's solution: // https://gist.github.com/wontondon/e8c4bdf2888875e4c755712e99279536 @@ -174,13 +175,14 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio return origUseRoutes; } - let isMountRenderPass: boolean = true; + const allRoutes: RouteObject[] = []; const SentryRoutes: React.FC<{ children?: React.ReactNode; routes: RouteObject[]; locationArg?: Partial | string; }> = (props: { children?: React.ReactNode; routes: RouteObject[]; locationArg?: Partial | string }) => { + const isMountRenderPass = React.useRef(true); const { routes, locationArg } = props; const Routes = origUseRoutes(routes, locationArg); @@ -198,11 +200,15 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio const normalizedLocation = typeof stableLocationParam === 'string' ? { pathname: stableLocationParam } : stableLocationParam; - if (isMountRenderPass) { - updatePageloadTransaction(getActiveRootSpan(), normalizedLocation, routes); - isMountRenderPass = false; + routes.forEach(route => { + allRoutes.push(...getChildRoutesRecursively(route)); + }); + + if (isMountRenderPass.current) { + updatePageloadTransaction(getActiveRootSpan(), normalizedLocation, routes, undefined, undefined, allRoutes); + isMountRenderPass.current = false; } else { - handleNavigation(normalizedLocation, routes, navigationType, version); + handleNavigation(normalizedLocation, routes, navigationType, version, undefined, undefined, allRoutes); } }, [navigationType, stableLocationParam]); @@ -222,6 +228,7 @@ export function handleNavigation( version: V6CompatibleVersion, matches?: AgnosticDataRouteMatch, basename?: string, + allRoutes?: RouteObject[], ): void { const branches = Array.isArray(matches) ? matches : _matchRoutes(routes, location, basename); @@ -233,8 +240,14 @@ export function handleNavigation( if ((navigationType === 'PUSH' || navigationType === 'POP') && branches) { const [name, source] = getNormalizedName(routes, location, branches, basename); + let txnName = name; + + if (locationIsInsideDescendantRoute(location, allRoutes || routes)) { + txnName = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); + } + startBrowserTracingNavigationSpan(client, { - name, + name: txnName, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', @@ -286,12 +299,93 @@ function sendIndexPath(pathBuilder: string, pathname: string, basename: string): return [formattedPath, 'route']; } -function pathEndsWithWildcard(path: string, branch: RouteMatch): boolean { - return (path.slice(-2) === '/*' && branch.route.children && branch.route.children.length > 0) || false; +function pathEndsWithWildcard(path: string): boolean { + return path.endsWith('*'); } function pathIsWildcardAndHasChildren(path: string, branch: RouteMatch): boolean { - return (path === '*' && branch.route.children && branch.route.children.length > 0) || false; + return (pathEndsWithWildcard(path) && branch.route.children && branch.route.children.length > 0) || false; +} + +// function pathIsWildcardWithNoChildren(path: string, branch: RouteMatch): boolean { +// return (pathEndsWithWildcard(path) && (!branch.route.children || branch.route.children.length === 0)) || false; +// } + +function routeIsDescendant(route: RouteObject): boolean { + return !!(!route.children && route.element && route.path && route.path.endsWith('/*')); +} + +function locationIsInsideDescendantRoute(location: Location, routes: RouteObject[]): boolean { + const matchedRoutes = _matchRoutes(routes, location) as RouteMatch[]; + + if (matchedRoutes) { + for (const match of matchedRoutes) { + if (routeIsDescendant(match.route) && pickSplat(match)) { + return true; + } + } + } + + return false; +} + +function getChildRoutesRecursively(route: RouteObject, allRoutes: RouteObject[] = []): RouteObject[] { + if (route.children && !route.index) { + route.children.forEach(child => { + allRoutes.push(...getChildRoutesRecursively(child, allRoutes)); + }); + } + + allRoutes.push(route); + + return allRoutes; +} + +function pickPath(match: RouteMatch): string { + return trimWildcard(match.route.path || ''); +} + +function pickSplat(match: RouteMatch): string { + return match.params['*'] || ''; +} + +function trimWildcard(path: string): string { + return path[path.length - 1] === '*' ? path.slice(0, -1) : path; +} + +function trimSlash(path: string): string { + return path[path.length - 1] === '/' ? path.slice(0, -1) : path; +} + +function prefixWithSlash(path: string): string { + return path[0] === '/' ? path : `/${path}`; +} + +function rebuildRoutePathFromAllRoutes(allRoutes: RouteObject[], location: Location): string { + const matchedRoutes = _matchRoutes(allRoutes, location) as RouteMatch[]; + + if (matchedRoutes) { + for (const match of matchedRoutes) { + if (match.route.path && match.route.path !== '*') { + const path = pickPath(match); + const strippedPath = stripBasenameFromPathname(location.pathname, prefixWithSlash(match.pathnameBase)); + + return trimSlash( + trimSlash(path || '') + + prefixWithSlash( + rebuildRoutePathFromAllRoutes( + allRoutes.filter(route => route !== match.route), + { + pathname: strippedPath, + }, + ), + ), + ); + } + } + } + + return ''; } function getNormalizedName( @@ -321,7 +415,10 @@ function getNormalizedName( pathBuilder += newPath; // If the path matches the current location, return the path - if (basename + branch.pathname === location.pathname) { + if ( + location.pathname.endsWith(basename + branch.pathname) || + location.pathname.endsWith(`${basename}${branch.pathname}/`) + ) { if ( // If the route defined on the element is something like // Product} /> @@ -330,13 +427,13 @@ function getNormalizedName( // eslint-disable-next-line deprecation/deprecation getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname) && // We should not count wildcard operators in the url segments calculation - pathBuilder.slice(-2) !== '/*' + !pathEndsWithWildcard(pathBuilder) ) { return [(_stripBasename ? '' : basename) + newPath, 'route']; } // if the last character of the pathbuilder is a wildcard and there are children, remove the wildcard - if (pathEndsWithWildcard(pathBuilder, branch)) { + if (pathIsWildcardAndHasChildren(pathBuilder, branch)) { pathBuilder = pathBuilder.slice(0, -1); } @@ -347,7 +444,11 @@ function getNormalizedName( } } - return [_stripBasename ? stripBasenameFromPathname(location.pathname, basename) : location.pathname, 'url']; + const fallbackTransactionName = _stripBasename + ? stripBasenameFromPathname(location.pathname, basename) + : location.pathname || '/'; + + return [fallbackTransactionName, 'url']; } function updatePageloadTransaction( @@ -356,6 +457,7 @@ function updatePageloadTransaction( routes: RouteObject[], matches?: AgnosticDataRouteMatch, basename?: string, + allRoutes?: RouteObject[], ): void { const branches = Array.isArray(matches) ? matches @@ -364,10 +466,16 @@ function updatePageloadTransaction( if (branches) { const [name, source] = getNormalizedName(routes, location, branches, basename); - getCurrentScope().setTransactionName(name); + let txnName = name; + + if (locationIsInsideDescendantRoute(location, allRoutes || routes)) { + txnName = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); + } + + getCurrentScope().setTransactionName(txnName); if (activeRootSpan) { - activeRootSpan.updateName(name); + activeRootSpan.updateName(txnName); activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); } } @@ -387,9 +495,11 @@ export function createV6CompatibleWithSentryReactRouterRouting

= (props: P) => { + const isMountRenderPass = React.useRef(true); + const location = _useLocation(); const navigationType = _useNavigationType(); @@ -397,11 +507,15 @@ export function createV6CompatibleWithSentryReactRouterRouting

{ const routes = _createRoutesFromChildren(props.children) as RouteObject[]; - if (isMountRenderPass) { - updatePageloadTransaction(getActiveRootSpan(), location, routes); - isMountRenderPass = false; + routes.forEach(route => { + allRoutes.push(...getChildRoutesRecursively(route)); + }); + + if (isMountRenderPass.current) { + updatePageloadTransaction(getActiveRootSpan(), location, routes, undefined, undefined, allRoutes); + isMountRenderPass.current = false; } else { - handleNavigation(location, routes, navigationType, version); + handleNavigation(location, routes, navigationType, version, undefined, undefined, allRoutes); } }, // `props.children` is purposely not included in the dependency array, because we do not want to re-run this effect diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx index 81afc2933861..9fa716bd3390 100644 --- a/packages/react/src/reactrouterv6.tsx +++ b/packages/react/src/reactrouterv6.tsx @@ -1,3 +1,7 @@ +/* eslint-disable complexity */ +/* eslint-disable max-lines */ +// Inspired from Donnie McNeal's solution: +// https://gist.github.com/wontondon/e8c4bdf2888875e4c755712e99279536 import type { browserTracingIntegration } from '@sentry/browser'; import type { Integration } from '@sentry/core'; import type { ReactRouterOptions } from './reactrouterv6-compat-utils'; diff --git a/packages/react/test/reactrouterv6.4.test.tsx b/packages/react/test/reactrouterv6.4.test.tsx index 3ae6a69bdf56..476e73425db3 100644 --- a/packages/react/test/reactrouterv6.4.test.tsx +++ b/packages/react/test/reactrouterv6.4.test.tsx @@ -17,7 +17,7 @@ import { matchRoutes, useLocation, useNavigationType, -} from 'react-router-6.4'; +} from 'react-router-6'; import { BrowserClient, wrapCreateBrowserRouter } from '../src'; import { reactRouterV6BrowserTracingIntegration } from '../src/reactrouterv6'; diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index b9cf4003c330..752b6c2aecf4 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -491,6 +491,55 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }); }); + it('works with descendant wildcard routes', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + const SentryRoutes = withSentryReactRouterV6Routing(Routes); + + const ProjectsRoutes = () => ( + + Project Page}> + Project Page Root} /> + Editor}> + }> + View Canvas} /> + + + + No Match Page} /> + + ); + + render( + + + } /> + }> + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/projects/:projectId/views/:viewId', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + }); + it("updates the scope's `transactionName` on a navigation", () => { const client = createMockBrowserClient(); setCurrentClient(client); From 86a3e65c759de36372c96a82862e97f790c0425f Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 22 Nov 2024 10:25:03 +0000 Subject: [PATCH 02/13] Add an E2E test. --- .../.gitignore | 29 ++++++++ .../react-router-6-descendant-routes/.npmrc | 2 + .../package.json | 63 +++++++++++++++++ .../playwright.config.mjs | 7 ++ .../public/index.html | 24 +++++++ .../server/app.js | 47 +++++++++++++ .../src/globals.d.ts | 5 ++ .../src/index.tsx | 68 +++++++++++++++++++ .../src/pages/Index.tsx | 23 +++++++ .../src/react-app-env.d.ts | 1 + .../start-event-proxy.mjs | 6 ++ .../tests/transactions.test.ts | 59 ++++++++++++++++ .../tsconfig.json | 20 ++++++ 13 files changed, 354 insertions(+) create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.gitignore create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.npmrc create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/playwright.config.mjs create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/public/index.html create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/server/app.js create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/globals.d.ts create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/react-app-env.d.ts create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/start-event-proxy.mjs create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts create mode 100644 dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tsconfig.json diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.gitignore b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.gitignore new file mode 100644 index 000000000000..84634c973eeb --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.gitignore @@ -0,0 +1,29 @@ +# See https://help.github.com/articles/ignoring-files/ for more about ignoring files. + +# dependencies +/node_modules +/.pnp +.pnp.js + +# testing +/coverage + +# production +/build + +# misc +.DS_Store +.env.local +.env.development.local +.env.test.local +.env.production.local + +npm-debug.log* +yarn-debug.log* +yarn-error.log* + +/test-results/ +/playwright-report/ +/playwright/.cache/ + +!*.d.ts diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.npmrc b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.npmrc new file mode 100644 index 000000000000..070f80f05092 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/.npmrc @@ -0,0 +1,2 @@ +@sentry:registry=http://127.0.0.1:4873 +@sentry-internal:registry=http://127.0.0.1:4873 diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json new file mode 100644 index 000000000000..2dfa406daecc --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json @@ -0,0 +1,63 @@ +{ + "name": "react-router-6-descendant-routes", + "version": "0.1.0", + "private": true, + "dependencies": { + "@sentry/react": "latest || *", + "@types/react": "18.0.0", + "@types/react-dom": "18.0.0", + "express": "4.19.2", + "react": "18.2.0", + "react-dom": "18.2.0", + "react-router-dom": "^6.28.0", + "react-scripts": "5.0.1", + "typescript": "4.9.5" + }, + "scripts": { + "build": "react-scripts build", + "start": "run-p start:client start:server", + "start:client": "node server/app.js", + "start:server": "serve -s build", + "test": "playwright test", + "clean": "npx rimraf node_modules pnpm-lock.yaml", + "test:build": "pnpm install && npx playwright install && pnpm build", + "test:build-ts3.8": "pnpm install && pnpm add typescript@3.8 && npx playwright install && pnpm build", + "test:build-canary": "pnpm install && pnpm add react@canary react-dom@canary && npx playwright install && pnpm build", + "test:assert": "pnpm test" + }, + "eslintConfig": { + "extends": [ + "react-app", + "react-app/jest" + ] + }, + "browserslist": { + "production": [ + ">0.2%", + "not dead", + "not op_mini all" + ], + "development": [ + "last 1 chrome version", + "last 1 firefox version", + "last 1 safari version" + ] + }, + "devDependencies": { + "@playwright/test": "^1.44.1", + "@sentry-internal/test-utils": "link:../../../test-utils", + "serve": "14.0.1", + "npm-run-all2": "^6.2.0" + }, + "volta": { + "extends": "../../package.json" + }, + "sentryTest": { + "variants": [ + { + "build-command": "test:build-ts3.8", + "label": "react-router-6 (TS 3.8)" + } + ] + } +} diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/playwright.config.mjs b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/playwright.config.mjs new file mode 100644 index 000000000000..31f2b913b58b --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/playwright.config.mjs @@ -0,0 +1,7 @@ +import { getPlaywrightConfig } from '@sentry-internal/test-utils'; + +const config = getPlaywrightConfig({ + startCommand: `pnpm start`, +}); + +export default config; diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/public/index.html b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/public/index.html new file mode 100644 index 000000000000..39da76522bea --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/public/index.html @@ -0,0 +1,24 @@ + + + + + + + + React App + + + +

+ + + diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/server/app.js b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/server/app.js new file mode 100644 index 000000000000..5a8cdb3929a1 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/server/app.js @@ -0,0 +1,47 @@ +const express = require('express'); + +const app = express(); +const PORT = 8080; + +const wait = time => { + return new Promise(resolve => { + setTimeout(() => { + resolve(); + }, time); + }); +}; + +async function sseHandler(request, response, timeout = false) { + response.headers = { + 'Content-Type': 'text/event-stream', + Connection: 'keep-alive', + 'Cache-Control': 'no-cache', + 'Access-Control-Allow-Origin': '*', + }; + + response.setHeader('Cache-Control', 'no-cache'); + response.setHeader('Content-Type', 'text/event-stream'); + response.setHeader('Access-Control-Allow-Origin', '*'); + response.setHeader('Connection', 'keep-alive'); + + response.flushHeaders(); + + await wait(2000); + + for (let index = 0; index < 10; index++) { + response.write(`data: ${new Date().toISOString()}\n\n`); + if (timeout) { + await wait(10000); + } + } + + response.end(); +} + +app.get('/sse', (req, res) => sseHandler(req, res)); + +app.get('/sse-timeout', (req, res) => sseHandler(req, res, true)); + +app.listen(PORT, () => { + console.log(`SSE service listening at http://localhost:${PORT}`); +}); diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/globals.d.ts b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/globals.d.ts new file mode 100644 index 000000000000..ffa61ca49acc --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/globals.d.ts @@ -0,0 +1,5 @@ +interface Window { + recordedTransactions?: string[]; + capturedExceptionId?: string; + sentryReplayId?: string; +} diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx new file mode 100644 index 000000000000..a85498e2402d --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx @@ -0,0 +1,68 @@ +import * as Sentry from '@sentry/react'; +import React from 'react'; +import ReactDOM from 'react-dom/client'; +import { + BrowserRouter, + Route, + Routes, + createRoutesFromChildren, + matchRoutes, + useLocation, + useNavigationType, + Navigate, + Outlet, +} from 'react-router-dom'; + +const replay = Sentry.replayIntegration(); + +Sentry.init({ + environment: 'qa', // dynamic sampling bias to keep transactions + dsn: process.env.REACT_APP_E2E_TEST_DSN, + integrations: [ + Sentry.reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + trackFetchStreamPerformance: true, + }), + replay, + ], + // We recommend adjusting this value in production, or using tracesSampler + // for finer control + tracesSampleRate: 1.0, + release: 'e2e-test', + + // Always capture replays, so we can test this properly + replaysSessionSampleRate: 1.0, + replaysOnErrorSampleRate: 0.0, + + tunnel: 'http://localhost:3031', +}); + +const SentryRoutes = Sentry.withSentryReactRouterV6Routing(Routes); + +const ProjectsRoutes = () => ( + + Project Page}> + Project Page Root} /> + Editor}> + }> + View Canvas} /> + + + + No Match Page} /> + +); + +const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement); +root.render( + + + {/* } /> */} + }> + + , +); diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx new file mode 100644 index 000000000000..d6b71a1d1279 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx @@ -0,0 +1,23 @@ +// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX +import * as React from 'react'; +import { Link } from 'react-router-dom'; + +const Index = () => { + return ( + <> + { + throw new Error('I am an error!'); + }} + /> + + navigate + + + ); +}; + +export default Index; diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/react-app-env.d.ts b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/react-app-env.d.ts new file mode 100644 index 000000000000..6431bc5fc6b2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/react-app-env.d.ts @@ -0,0 +1 @@ +/// diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/start-event-proxy.mjs b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/start-event-proxy.mjs new file mode 100644 index 000000000000..abd4db1ea605 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/start-event-proxy.mjs @@ -0,0 +1,6 @@ +import { startEventProxyServer } from '@sentry-internal/test-utils'; + +startEventProxyServer({ + port: 3031, + proxyServerName: 'react-router-6-descendant-routes', +}); diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts new file mode 100644 index 000000000000..de19ceac638f --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts @@ -0,0 +1,59 @@ +import { expect, test } from '@playwright/test'; +import { waitForTransaction } from '@sentry-internal/test-utils'; + +test('sends a pageload transaction with a parameterized URL', async ({ page }) => { + const transactionPromise = waitForTransaction('react-router-6-descendant-routes', async transactionEvent => { + return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload'; + }); + + await page.goto(`/projects/123/views/234`); + + const rootSpan = await transactionPromise; + + console.debug('rootSpan', rootSpan); + + expect(rootSpan).toMatchObject({ + contexts: { + trace: { + op: 'pageload', + origin: 'auto.pageload.react.reactrouter_v6', + }, + }, + transaction: '/projects/:projectId/views/:viewId', + transaction_info: { + source: 'route', + }, + }); +}); + +// test('sends a navigation transaction with a parameterized URL', async ({ page }) => { +// page.on('console', msg => console.log(msg.text())); + +// 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(`/`); +// await pageloadTxnPromise; + +// const linkElement = page.locator('id=navigation'); + +// const [_, navigationTxn] = await Promise.all([linkElement.click(), navigationTxnPromise]); + +// expect(navigationTxn).toMatchObject({ +// contexts: { +// trace: { +// op: 'navigation', +// origin: 'auto.navigation.react.reactrouter_v6', +// }, +// }, +// transaction: '/user/:id', +// transaction_info: { +// source: 'route', +// }, +// }); +// }); diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tsconfig.json b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tsconfig.json new file mode 100644 index 000000000000..4cc95dc2689a --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tsconfig.json @@ -0,0 +1,20 @@ +{ + "compilerOptions": { + "target": "es2018", + "lib": ["dom", "dom.iterable", "esnext"], + "allowJs": true, + "skipLibCheck": true, + "esModuleInterop": true, + "allowSyntheticDefaultImports": true, + "strict": true, + "forceConsistentCasingInFileNames": true, + "noFallthroughCasesInSwitch": true, + "module": "esnext", + "moduleResolution": "node", + "resolveJsonModule": true, + "isolatedModules": true, + "noEmit": true, + "jsx": "react" + }, + "include": ["src", "tests"] +} From 05213995e5f8e914e339eab6fecb09f956b01dd9 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 22 Nov 2024 10:26:46 +0000 Subject: [PATCH 03/13] Alternative implementation. --- packages/react/test/reactrouterv6.test.tsx | 50 +++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index 752b6c2aecf4..af296966d7bd 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -491,7 +491,55 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }); }); - it('works with descendant wildcard routes', () => { + it('works with descendant wildcard routes - pageload', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + const SentryRoutes = withSentryReactRouterV6Routing(Routes); + + const ProjectsRoutes = () => ( + + Project Page}> + Project Page Root} /> + Editor}> + }> + View Canvas} /> + + + + No Match Page} /> + + ); + + render( + + + }> + + , + ); + + expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/projects/:projectId/views/:viewId', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.react.reactrouter_v6', + }, + }); + }); + + it('works with descendant wildcard routes - navigation', () => { const client = createMockBrowserClient(); setCurrentClient(client); From e20f2197982a0d401898e1339c83608a1b3c947b Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 28 Nov 2024 20:14:46 +0000 Subject: [PATCH 04/13] Switch back to the original implementation. --- packages/react/test/reactrouterv6.test.tsx | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index af296966d7bd..4f4262606899 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -529,14 +529,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { ); expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/projects/:projectId/views/:viewId', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.react.reactrouter_v6', - }, - }); + expect(mockRootSpan.updateName).toHaveBeenLastCalledWith('/projects/:projectId/views/:viewId'); + expect(mockRootSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); it('works with descendant wildcard routes - navigation', () => { @@ -565,6 +559,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => {
No Match Page} /> + 404} /> ); From 0ebdf39426ac4cfcffd44c8a38500d2e6b968555 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 28 Nov 2024 20:26:33 +0000 Subject: [PATCH 05/13] Lint --- .../react-router-6-descendant-routes/src/index.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx index a85498e2402d..a2bf5210c455 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx @@ -3,14 +3,13 @@ import React from 'react'; import ReactDOM from 'react-dom/client'; import { BrowserRouter, + Outlet, Route, Routes, createRoutesFromChildren, matchRoutes, useLocation, useNavigationType, - Navigate, - Outlet, } from 'react-router-dom'; const replay = Sentry.replayIntegration(); From 23b8c36380031fd3bcebca230650041edb96f63b Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 28 Nov 2024 21:02:52 +0000 Subject: [PATCH 06/13] Update e2e tests. --- .../package.json | 8 -- .../src/index.tsx | 3 +- .../src/pages/Index.tsx | 10 +-- .../tests/transactions.test.ts | 74 ++++++++++--------- 4 files changed, 44 insertions(+), 51 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json index 2dfa406daecc..4fc913833ce5 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/package.json @@ -51,13 +51,5 @@ }, "volta": { "extends": "../../package.json" - }, - "sentryTest": { - "variants": [ - { - "build-command": "test:build-ts3.8", - "label": "react-router-6 (TS 3.8)" - } - ] } } diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx index a2bf5210c455..a1a9066147d1 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx @@ -11,6 +11,7 @@ import { useLocation, useNavigationType, } from 'react-router-dom'; +import Index from './pages/Index'; const replay = Sentry.replayIntegration(); @@ -60,7 +61,7 @@ const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement) root.render( - {/* } /> */} + } /> }> , diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx index d6b71a1d1279..3b618e34fe2c 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx @@ -5,15 +5,7 @@ import { Link } from 'react-router-dom'; const Index = () => { return ( <> - { - throw new Error('I am an error!'); - }} - /> - + navigate diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts index de19ceac638f..242acf754e00 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts @@ -10,8 +10,6 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) = const rootSpan = await transactionPromise; - console.debug('rootSpan', rootSpan); - expect(rootSpan).toMatchObject({ contexts: { trace: { @@ -26,34 +24,44 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) = }); }); -// test('sends a navigation transaction with a parameterized URL', async ({ page }) => { -// page.on('console', msg => console.log(msg.text())); - -// 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(`/`); -// await pageloadTxnPromise; - -// const linkElement = page.locator('id=navigation'); - -// const [_, navigationTxn] = await Promise.all([linkElement.click(), navigationTxnPromise]); - -// expect(navigationTxn).toMatchObject({ -// contexts: { -// trace: { -// op: 'navigation', -// origin: 'auto.navigation.react.reactrouter_v6', -// }, -// }, -// transaction: '/user/:id', -// 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'; + }); + + 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=navigation'); + + const [_, navigationTxn] = await Promise.all([linkElement.click(), navigationTxnPromise]); + expect(navigationTxn).toMatchObject({ + contexts: { + trace: { + op: 'navigation', + origin: 'auto.navigation.react.reactrouter_v6', + }, + }, + transaction: '/projects/:projectId/views/:viewId', + transaction_info: { + source: 'route', + }, + }); +}); From a39c8ffa0cb8a59260b8632c0409f35f53102be7 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 28 Nov 2024 21:15:01 +0000 Subject: [PATCH 07/13] Switch to `useRef` in `wrapUseRoutes`. --- .../react-router-6-descendant-routes/src/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx index a1a9066147d1..a1c4942306e2 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx @@ -61,7 +61,7 @@ const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement) root.render( - } /> + } /> }> , From d9f99629406c14bb39e22b9e530e54b788ff7ce8 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 3 Dec 2024 12:15:03 +0000 Subject: [PATCH 08/13] Reimplement descendant routes handling --- packages/react/package.json | 3 +- packages/react/test/reactrouterv6.4.test.tsx | 676 ------------------- packages/react/test/reactrouterv6.test.tsx | 218 ++++-- yarn.lock | 25 +- 4 files changed, 189 insertions(+), 733 deletions(-) delete mode 100644 packages/react/test/reactrouterv6.4.test.tsx diff --git a/packages/react/package.json b/packages/react/package.json index 938714906a2a..19dc93d2fd22 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -67,8 +67,7 @@ "react-router-3": "npm:react-router@3.2.0", "react-router-4": "npm:react-router@4.1.0", "react-router-5": "npm:react-router@5.0.0", - "react-router-6": "npm:react-router@6.3.0", - "react-router-6.4": "npm:react-router@6.4.2", + "react-router-6": "npm:react-router@6.28.0", "redux": "^4.0.5" }, "scripts": { diff --git a/packages/react/test/reactrouterv6.4.test.tsx b/packages/react/test/reactrouterv6.4.test.tsx deleted file mode 100644 index 476e73425db3..000000000000 --- a/packages/react/test/reactrouterv6.4.test.tsx +++ /dev/null @@ -1,676 +0,0 @@ -import { - SEMANTIC_ATTRIBUTE_SENTRY_OP, - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - createTransport, - getCurrentScope, - setCurrentClient, -} from '@sentry/core'; -import { render } from '@testing-library/react'; -import { Request } from 'node-fetch'; -import * as React from 'react'; -import { - Navigate, - RouterProvider, - createMemoryRouter, - createRoutesFromChildren, - matchRoutes, - useLocation, - useNavigationType, -} from 'react-router-6'; - -import { BrowserClient, wrapCreateBrowserRouter } from '../src'; -import { reactRouterV6BrowserTracingIntegration } from '../src/reactrouterv6'; -import type { CreateRouterFunction } from '../src/types'; - -beforeAll(() => { - // @ts-expect-error need to override global Request because it's not in the jest environment (even with an - // `@jest-environment jsdom` directive, for some reason) - global.Request = Request; -}); - -const mockStartBrowserTracingPageLoadSpan = jest.fn(); -const mockStartBrowserTracingNavigationSpan = jest.fn(); - -const mockRootSpan = { - updateName: jest.fn(), - setAttribute: jest.fn(), - getSpanJSON() { - return { op: 'pageload' }; - }, -}; - -jest.mock('@sentry/browser', () => { - const actual = jest.requireActual('@sentry/browser'); - return { - ...actual, - startBrowserTracingNavigationSpan: (...args: unknown[]) => { - mockStartBrowserTracingNavigationSpan(...args); - return actual.startBrowserTracingNavigationSpan(...args); - }, - startBrowserTracingPageLoadSpan: (...args: unknown[]) => { - mockStartBrowserTracingPageLoadSpan(...args); - return actual.startBrowserTracingPageLoadSpan(...args); - }, - }; -}); - -jest.mock('@sentry/core', () => { - const actual = jest.requireActual('@sentry/core'); - return { - ...actual, - getRootSpan: () => { - return mockRootSpan; - }, - }; -}); - -describe('reactRouterV6BrowserTracingIntegration (v6.4)', () => { - function createMockBrowserClient(): BrowserClient { - return new BrowserClient({ - integrations: [], - tracesSampleRate: 1, - transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => Promise.resolve({})), - stackParser: () => [], - }); - } - - beforeEach(() => { - jest.clearAllMocks(); - getCurrentScope().setClient(undefined); - }); - - describe('wrapCreateBrowserRouter', () => { - it('starts a pageload transaction', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element:
TEST
, - }, - ], - { - initialEntries: ['/'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.react.reactrouter_v6', - }, - }); - }); - - it("updates the scope's `transactionName` on a pageload", () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element:
TEST
, - }, - ], - { - initialEntries: ['/'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(getCurrentScope().getScopeData().transactionName).toEqual('/'); - }); - - it('starts a navigation transaction', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: 'about', - element:
About
, - }, - ], - { - initialEntries: ['/'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/about', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it('works with nested routes', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: 'about', - element:
About
, - children: [ - { - path: 'us', - element:
Us
, - }, - ], - }, - ], - { - initialEntries: ['/'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/about/us', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it('works with parameterized paths', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: 'about', - element:
About
, - children: [ - { - path: ':page', - element:
Page
, - }, - ], - }, - ], - { - initialEntries: ['/'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/about/:page', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it('works with paths with multiple parameters', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: 'stores', - element:
Stores
, - children: [ - { - path: ':storeId', - element:
Store
, - children: [ - { - path: 'products', - element:
Products
, - children: [ - { - path: ':productId', - element:
Product
, - }, - ], - }, - ], - }, - ], - }, - ], - { - initialEntries: ['/'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/stores/:storeId/products/:productId', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it('updates pageload transaction to a parameterized route', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: 'about', - element:
About
, - children: [ - { - path: ':page', - element:
page
, - }, - ], - }, - ], - { - initialEntries: ['/about/us'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenCalledTimes(1); - expect(mockRootSpan.updateName).toHaveBeenLastCalledWith('/about/:page'); - expect(mockRootSpan.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); - }); - - it('works with `basename` option', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: 'about', - element:
About
, - children: [ - { - path: 'us', - element:
Us
, - }, - ], - }, - ], - { - initialEntries: ['/app'], - basename: '/app', - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/app/about/us', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it('works with parameterized paths and `basename`', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: ':orgId', - children: [ - { - path: 'users', - children: [ - { - path: ':userId', - element:
User
, - }, - ], - }, - ], - }, - ], - { - initialEntries: ['/admin'], - basename: '/admin', - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/admin/:orgId/users/:userId', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it('strips `basename` from transaction names of parameterized paths', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - stripBasename: true, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: ':orgId', - children: [ - { - path: 'users', - children: [ - { - path: ':userId', - element:
User
, - }, - ], - }, - ], - }, - ], - { - initialEntries: ['/admin'], - basename: '/admin', - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/:orgId/users/:userId', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it('strips `basename` from transaction names of non-parameterized paths', () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - stripBasename: true, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: 'about', - element:
About
, - children: [ - { - path: 'us', - element:
Us
, - }, - ], - }, - ], - { - initialEntries: ['/app'], - basename: '/app', - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); - expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/about/us', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', - }, - }); - }); - - it("updates the scope's `transactionName` on a navigation", () => { - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.addIntegration( - reactRouterV6BrowserTracingIntegration({ - useEffect: React.useEffect, - useLocation, - useNavigationType, - createRoutesFromChildren, - matchRoutes, - }), - ); - // eslint-disable-next-line deprecation/deprecation - const sentryCreateBrowserRouter = wrapCreateBrowserRouter(createMemoryRouter as CreateRouterFunction); - - const router = sentryCreateBrowserRouter( - [ - { - path: '/', - element: , - }, - { - path: 'about', - element:
About
, - }, - ], - { - initialEntries: ['/'], - }, - ); - - // @ts-expect-error router is fine - render(); - - expect(getCurrentScope().getScopeData().transactionName).toEqual('/about'); - }); - }); -}); diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index 4f4262606899..1d44d8cf801a 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -25,7 +25,7 @@ import { BrowserClient } from '../src'; import { reactRouterV6BrowserTracingIntegration, withSentryReactRouterV6Routing, - wrapUseRoutes, + wrapUseRoutesV6, } from '../src/reactrouterv6'; const mockStartBrowserTracingPageLoadSpan = jest.fn(); @@ -506,30 +506,36 @@ describe('reactRouterV6BrowserTracingIntegration', () => { ); const SentryRoutes = withSentryReactRouterV6Routing(Routes); + const DetailsRoutes = () => ( + + Details} /> + + ); + + const ViewsRoutes = () => ( + + Views} /> + } /> + + ); + const ProjectsRoutes = () => ( - Project Page}> - Project Page Root} /> - Editor}> - }> - View Canvas} /> - - - + }> No Match Page} /> ); render( - + - }> + }> , ); expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenCalledTimes(1); - expect(mockRootSpan.updateName).toHaveBeenLastCalledWith('/projects/:projectId/views/:viewId'); + expect(mockRootSpan.updateName).toHaveBeenLastCalledWith('/projects/:projectId/views/:viewId/:detailId'); expect(mockRootSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); @@ -613,7 +619,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }); }); - describe('wrapUseRoutes', () => { + describe('wrapUseRoutesV6', () => { it('starts a pageload transaction', () => { const client = createMockBrowserClient(); setCurrentClient(client); @@ -628,8 +634,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -670,8 +675,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -705,8 +709,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -740,8 +743,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -778,8 +780,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -823,8 +825,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -874,8 +876,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -925,8 +927,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -982,8 +984,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -1063,8 +1065,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -1129,6 +1131,144 @@ describe('reactRouterV6BrowserTracingIntegration', () => { }); }); + it('works with descendant wildcard routes - pageload', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); + + const ProjectsRoutes = () => + wrappedUseRoutes([ + { + path: ':projectId', + element:
Project Page
, + children: [ + { + index: true, + element:
Project Page Root
, + }, + { + element:
Editor
, + children: [ + { + path: '*', + element: , + children: [ + { + path: 'views/:viewId/*', + element:
View Canvas
, + }, + ], + }, + ], + }, + ], + }, + ]); + + const Routes = () => + wrappedUseRoutes([ + { + path: 'projects/*', + element: , + }, + ]); + + render( + + + , + ); + + expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenCalledTimes(1); + expect(mockRootSpan.updateName).toHaveBeenLastCalledWith('/projects/:projectId/views/:viewId'); + expect(mockRootSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); + }); + + it('works with descendant wildcard routes - navigation', () => { + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.addIntegration( + reactRouterV6BrowserTracingIntegration({ + useEffect: React.useEffect, + useLocation, + useNavigationType, + createRoutesFromChildren, + matchRoutes, + }), + ); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); + + const ProjectsRoutes = () => + wrappedUseRoutes([ + { + path: ':projectId', + element:
Project Page
, + children: [ + { + index: true, + element:
Project Page Root
, + }, + { + element:
Editor
, + children: [ + { + path: '*', + element: , + children: [ + { + path: 'views/:viewId/*', + element:
View Canvas
, + }, + ], + }, + ], + }, + ], + }, + ]); + + const Routes = () => + wrappedUseRoutes([ + { + index: true, + element: , + }, + { + path: 'projects/*', + element: , + }, + ]); + + render( + + + , + ); + + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); + expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { + name: '/projects/:projectId/views/:viewId', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.react.reactrouter_v6', + }, + }); + }); + it('does not add double slashes to URLS', () => { const client = createMockBrowserClient(); setCurrentClient(client); @@ -1142,8 +1282,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -1201,8 +1341,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ @@ -1259,8 +1399,8 @@ describe('reactRouterV6BrowserTracingIntegration', () => { matchRoutes, }), ); - // eslint-disable-next-line deprecation/deprecation - const wrappedUseRoutes = wrapUseRoutes(useRoutes); + + const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); const Routes = () => wrappedUseRoutes([ diff --git a/yarn.lock b/yarn.lock index bbe8ef951e89..efbb796f198e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8172,10 +8172,10 @@ history "^5.3.0" react-router-dom "^6.2.2" -"@remix-run/router@1.0.2": - version "1.0.2" - resolved "https://registry.yarnpkg.com/@remix-run/router/-/router-1.0.2.tgz#1c17eadb2fa77f80a796ad5ea9bf108e6993ef06" - integrity sha512-GRSOFhJzjGN+d4sKHTMSvNeUPoZiDHWmRnXfzaxrqe7dE/Nzlc8BiMSJdLDESZlndM7jIUrZ/F4yWqVYlI0rwQ== +"@remix-run/router@1.21.0": + version "1.21.0" + resolved "https://registry.yarnpkg.com/@remix-run/router/-/router-1.21.0.tgz#c65ae4262bdcfe415dbd4f64ec87676e4a56e2b5" + integrity sha512-xfSkCAchbdG5PnbrKqFWwia4Bi61nH+wm8wLEqfHDyp7Y3dZzgqS2itV8i4gAq9pC2HsTpwyBC6Ds8VHZ96JlA== "@remix-run/router@1.x": version "1.15.0" @@ -28703,19 +28703,12 @@ react-is@^18.0.0: tiny-invariant "^1.0.2" tiny-warning "^1.0.0" -"react-router-6.4@npm:react-router@6.4.2": - version "6.4.2" - resolved "https://registry.yarnpkg.com/react-router/-/react-router-6.4.2.tgz#300628ee9ed81b8ef1597b5cb98b474efe9779b8" - integrity sha512-Rb0BAX9KHhVzT1OKhMvCDMw776aTYM0DtkxqUBP8dNBom3mPXlfNs76JNGK8wKJ1IZEY1+WGj+cvZxHVk/GiKw== +"react-router-6@npm:react-router@6.28.0": + version "6.28.0" + resolved "https://registry.yarnpkg.com/react-router/-/react-router-6.28.0.tgz#29247c86d7ba901d7e5a13aa79a96723c3e59d0d" + integrity sha512-HrYdIFqdrnhDw0PqG/AKjAqEqM7AvxCz0DQ4h2W8k6nqmc5uRBYDag0SBxx9iYz5G8gnuNVLzUe13wl9eAsXXg== dependencies: - "@remix-run/router" "1.0.2" - -"react-router-6@npm:react-router@6.3.0": - version "6.3.0" - resolved "https://registry.yarnpkg.com/react-router/-/react-router-6.3.0.tgz#3970cc64b4cb4eae0c1ea5203a80334fdd175557" - integrity sha512-7Wh1DzVQ+tlFjkeo+ujvjSqSJmkt1+8JO+T5xklPlgrh70y7ogx75ODRW0ThWhY7S+6yEDks8TYrtQe/aoboBQ== - dependencies: - history "^5.2.0" + "@remix-run/router" "1.21.0" react-router-dom@^6.2.2: version "6.3.0" From 37fafae2cd7b568492d416247a75623d3f142128 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 4 Dec 2024 14:31:36 +0000 Subject: [PATCH 09/13] Optimize --- .../react/src/reactrouterv6-compat-utils.tsx | 52 +++++++++++-------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 6cdde5c1e940..7635e610657e 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -200,11 +200,11 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio const normalizedLocation = typeof stableLocationParam === 'string' ? { pathname: stableLocationParam } : stableLocationParam; - routes.forEach(route => { - allRoutes.push(...getChildRoutesRecursively(route)); - }); - if (isMountRenderPass.current) { + routes.forEach(route => { + allRoutes.push(...getChildRoutesRecursively(route)); + }); + updatePageloadTransaction(getActiveRootSpan(), normalizedLocation, routes, undefined, undefined, allRoutes); isMountRenderPass.current = false; } else { @@ -238,16 +238,21 @@ export function handleNavigation( } if ((navigationType === 'PUSH' || navigationType === 'POP') && branches) { - const [name, source] = getNormalizedName(routes, location, branches, basename); + let name, + source: TransactionSource = 'url'; + const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes || routes); - let txnName = name; + if (isInDescendantRoute) { + name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); + source = 'route'; + } - if (locationIsInsideDescendantRoute(location, allRoutes || routes)) { - txnName = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); + if (!isInDescendantRoute || !name) { + [name, source] = getNormalizedName(routes, location, branches, basename); } startBrowserTracingNavigationSpan(client, { - name: txnName, + name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', @@ -307,10 +312,6 @@ function pathIsWildcardAndHasChildren(path: string, branch: RouteMatch): return (pathEndsWithWildcard(path) && branch.route.children && branch.route.children.length > 0) || false; } -// function pathIsWildcardWithNoChildren(path: string, branch: RouteMatch): boolean { -// return (pathEndsWithWildcard(path) && (!branch.route.children || branch.route.children.length === 0)) || false; -// } - function routeIsDescendant(route: RouteObject): boolean { return !!(!route.children && route.element && route.path && route.path.endsWith('/*')); } @@ -464,18 +465,23 @@ function updatePageloadTransaction( : (_matchRoutes(routes, location, basename) as unknown as RouteMatch[]); if (branches) { - const [name, source] = getNormalizedName(routes, location, branches, basename); + let name, + source: TransactionSource = 'url'; + const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes || routes); - let txnName = name; + if (isInDescendantRoute) { + name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); + source = 'route'; + } - if (locationIsInsideDescendantRoute(location, allRoutes || routes)) { - txnName = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); + if (!isInDescendantRoute || !name) { + [name, source] = getNormalizedName(routes, location, branches, basename); } - getCurrentScope().setTransactionName(txnName); + getCurrentScope().setTransactionName(name); if (activeRootSpan) { - activeRootSpan.updateName(txnName); + activeRootSpan.updateName(name); activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); } } @@ -507,11 +513,11 @@ export function createV6CompatibleWithSentryReactRouterRouting

{ const routes = _createRoutesFromChildren(props.children) as RouteObject[]; - routes.forEach(route => { - allRoutes.push(...getChildRoutesRecursively(route)); - }); - if (isMountRenderPass.current) { + routes.forEach(route => { + allRoutes.push(...getChildRoutesRecursively(route)); + }); + updatePageloadTransaction(getActiveRootSpan(), location, routes, undefined, undefined, allRoutes); isMountRenderPass.current = false; } else { From ac005f8334910702588b389a1175b29ec3d55106 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 4 Dec 2024 14:41:18 +0000 Subject: [PATCH 10/13] Cleanup --- packages/react/src/reactrouterv6-compat-utils.tsx | 6 +----- packages/react/src/reactrouterv6.tsx | 4 ---- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 7635e610657e..33a4e794a0c0 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -1,4 +1,3 @@ -/* eslint-disable complexity */ /* eslint-disable max-lines */ // Inspired from Donnie McNeal's solution: // https://gist.github.com/wontondon/e8c4bdf2888875e4c755712e99279536 @@ -416,10 +415,7 @@ function getNormalizedName( pathBuilder += newPath; // If the path matches the current location, return the path - if ( - location.pathname.endsWith(basename + branch.pathname) || - location.pathname.endsWith(`${basename}${branch.pathname}/`) - ) { + if (location.pathname.endsWith(basename + branch.pathname)) { if ( // If the route defined on the element is something like // Product} /> diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx index 9fa716bd3390..81afc2933861 100644 --- a/packages/react/src/reactrouterv6.tsx +++ b/packages/react/src/reactrouterv6.tsx @@ -1,7 +1,3 @@ -/* eslint-disable complexity */ -/* eslint-disable max-lines */ -// Inspired from Donnie McNeal's solution: -// https://gist.github.com/wontondon/e8c4bdf2888875e4c755712e99279536 import type { browserTracingIntegration } from '@sentry/browser'; import type { Integration } from '@sentry/core'; import type { ReactRouterOptions } from './reactrouterv6-compat-utils'; From 4ec53c8e9d4ae983da8255adb58d1ead502cee2a Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 4 Dec 2024 14:51:02 +0000 Subject: [PATCH 11/13] Update `useRoutes` tests. --- packages/react/test/reactrouterv6.test.tsx | 139 +++++++++++---------- 1 file changed, 75 insertions(+), 64 deletions(-) diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index 1d44d8cf801a..815b562f08f7 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -554,33 +554,38 @@ describe('reactRouterV6BrowserTracingIntegration', () => { ); const SentryRoutes = withSentryReactRouterV6Routing(Routes); + const DetailsRoutes = () => ( + + Details} /> + + ); + + const ViewsRoutes = () => ( + + Views} /> + } /> + + ); + const ProjectsRoutes = () => ( - Project Page}> - Project Page Root} /> - Editor}> - }> - View Canvas} /> - - - + }> No Match Page} /> - 404} /> ); render( - } /> - }> + } /> + }> , ); expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/projects/:projectId/views/:viewId', + name: '/projects/:projectId/views/:viewId/:detailId', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', @@ -1147,51 +1152,54 @@ describe('reactRouterV6BrowserTracingIntegration', () => { const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); + const DetailsRoutes = () => + wrappedUseRoutes([ + { + path: ':detailId', + element:

Details
, + }, + ]); + + const ViewsRoutes = () => + wrappedUseRoutes([ + { + index: true, + element:
Views
, + }, + { + path: 'views/:viewId/*', + element: , + }, + ]); + const ProjectsRoutes = () => wrappedUseRoutes([ { - path: ':projectId', - element:
Project Page
, - children: [ - { - index: true, - element:
Project Page Root
, - }, - { - element:
Editor
, - children: [ - { - path: '*', - element: , - children: [ - { - path: 'views/:viewId/*', - element:
View Canvas
, - }, - ], - }, - ], - }, - ], + path: 'projects/:projectId/*', + element: , + }, + { + path: '*', + element:
No Match Page
, }, ]); const Routes = () => wrappedUseRoutes([ { - path: 'projects/*', + path: '/*', element: , }, ]); render( - + , ); expect(mockStartBrowserTracingPageLoadSpan).toHaveBeenCalledTimes(1); - expect(mockRootSpan.updateName).toHaveBeenLastCalledWith('/projects/:projectId/views/:viewId'); + expect(mockRootSpan.updateName).toHaveBeenLastCalledWith('/projects/:projectId/views/:viewId/:detailId'); expect(mockRootSpan.setAttribute).toHaveBeenLastCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); }); @@ -1211,32 +1219,35 @@ describe('reactRouterV6BrowserTracingIntegration', () => { const wrappedUseRoutes = wrapUseRoutesV6(useRoutes); + const DetailsRoutes = () => + wrappedUseRoutes([ + { + path: ':detailId', + element:
Details
, + }, + ]); + + const ViewsRoutes = () => + wrappedUseRoutes([ + { + index: true, + element:
Views
, + }, + { + path: 'views/:viewId/*', + element: , + }, + ]); + const ProjectsRoutes = () => wrappedUseRoutes([ { - path: ':projectId', - element:
Project Page
, - children: [ - { - index: true, - element:
Project Page Root
, - }, - { - element:
Editor
, - children: [ - { - path: '*', - element: , - children: [ - { - path: 'views/:viewId/*', - element:
View Canvas
, - }, - ], - }, - ], - }, - ], + path: 'projects/:projectId/*', + element: , + }, + { + path: '*', + element:
No Match Page
, }, ]); @@ -1244,10 +1255,10 @@ describe('reactRouterV6BrowserTracingIntegration', () => { wrappedUseRoutes([ { index: true, - element: , + element: , }, { - path: 'projects/*', + path: '/*', element: , }, ]); @@ -1260,7 +1271,7 @@ describe('reactRouterV6BrowserTracingIntegration', () => { expect(mockStartBrowserTracingNavigationSpan).toHaveBeenCalledTimes(1); expect(mockStartBrowserTracingNavigationSpan).toHaveBeenLastCalledWith(expect.any(BrowserClient), { - name: '/projects/:projectId/views/:viewId', + name: '/projects/:projectId/views/:viewId/:detailId', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', From 89751f0c2f7ce05a7f1e7eee4372b25a05a15d74 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 4 Dec 2024 16:48:17 +0000 Subject: [PATCH 12/13] Update e2e tests. --- .../src/index.tsx | 25 +++++++++++-------- .../src/pages/Index.tsx | 2 +- .../tests/transactions.test.ts | 6 ++--- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx index a1c4942306e2..f6694a954915 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/index.tsx @@ -3,7 +3,6 @@ import React from 'react'; import ReactDOM from 'react-dom/client'; import { BrowserRouter, - Outlet, Route, Routes, createRoutesFromChildren, @@ -43,16 +42,22 @@ Sentry.init({ const SentryRoutes = Sentry.withSentryReactRouterV6Routing(Routes); +const DetailsRoutes = () => ( + + Details} /> + +); + +const ViewsRoutes = () => ( + + Views} /> + } /> + +); + const ProjectsRoutes = () => ( - Project Page}> - Project Page Root} /> - Editor}> - }> - View Canvas} /> - - - + }> No Match Page} /> ); @@ -62,7 +67,7 @@ root.render( } /> - }> + }> , ); diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx index 3b618e34fe2c..aa99b61f89ea 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/src/pages/Index.tsx @@ -5,7 +5,7 @@ import { Link } from 'react-router-dom'; const Index = () => { return ( <> - + navigate diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts index 242acf754e00..23bc0aaabe95 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-6-descendant-routes/tests/transactions.test.ts @@ -6,7 +6,7 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) = return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload'; }); - await page.goto(`/projects/123/views/234`); + await page.goto(`/projects/123/views/234/567`); const rootSpan = await transactionPromise; @@ -17,7 +17,7 @@ test('sends a pageload transaction with a parameterized URL', async ({ page }) = origin: 'auto.pageload.react.reactrouter_v6', }, }, - transaction: '/projects/:projectId/views/:viewId', + transaction: '/projects/:projectId/views/:viewId/:detailId', transaction_info: { source: 'route', }, @@ -59,7 +59,7 @@ test('sends a navigation transaction with a parameterized URL', async ({ page }) origin: 'auto.navigation.react.reactrouter_v6', }, }, - transaction: '/projects/:projectId/views/:viewId', + transaction: '/projects/:projectId/views/:viewId/:detailId', transaction_info: { source: 'route', }, From 6184b27b3964ea7c190661ff64067d24c0b0a1e8 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 11 Dec 2024 17:04:31 +0000 Subject: [PATCH 13/13] Review updates --- .../react/src/reactrouterv6-compat-utils.tsx | 80 ++++++++++++------- 1 file changed, 51 insertions(+), 29 deletions(-) diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 33a4e794a0c0..c17ea1bb190f 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -100,7 +100,13 @@ export function createV6CompatibleWrapCreateBrowserRouter< router.subscribe((state: RouterState) => { const location = state.location; if (state.historyAction === 'PUSH' || state.historyAction === 'POP') { - handleNavigation(location, routes, state.historyAction, version, undefined, basename); + handleNavigation({ + location, + routes, + navigationType: state.historyAction, + version, + basename, + }); } }); @@ -207,7 +213,13 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio updatePageloadTransaction(getActiveRootSpan(), normalizedLocation, routes, undefined, undefined, allRoutes); isMountRenderPass.current = false; } else { - handleNavigation(normalizedLocation, routes, navigationType, version, undefined, undefined, allRoutes); + handleNavigation({ + location: normalizedLocation, + routes, + navigationType, + version, + allRoutes, + }); } }, [navigationType, stableLocationParam]); @@ -220,15 +232,17 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio }; } -export function handleNavigation( - location: Location, - routes: RouteObject[], - navigationType: Action, - version: V6CompatibleVersion, - matches?: AgnosticDataRouteMatch, - basename?: string, - allRoutes?: RouteObject[], -): void { +export function handleNavigation(opts: { + location: Location; + routes: RouteObject[]; + navigationType: Action; + version: V6CompatibleVersion; + matches?: AgnosticDataRouteMatch; + basename?: string; + allRoutes?: RouteObject[]; +}): void { + const { location, routes, navigationType, version, matches, basename, allRoutes } = opts; + const branches = Array.isArray(matches) ? matches : _matchRoutes(routes, location, basename); const client = getClient(); @@ -364,24 +378,26 @@ function prefixWithSlash(path: string): string { function rebuildRoutePathFromAllRoutes(allRoutes: RouteObject[], location: Location): string { const matchedRoutes = _matchRoutes(allRoutes, location) as RouteMatch[]; - if (matchedRoutes) { - for (const match of matchedRoutes) { - if (match.route.path && match.route.path !== '*') { - const path = pickPath(match); - const strippedPath = stripBasenameFromPathname(location.pathname, prefixWithSlash(match.pathnameBase)); - - return trimSlash( - trimSlash(path || '') + - prefixWithSlash( - rebuildRoutePathFromAllRoutes( - allRoutes.filter(route => route !== match.route), - { - pathname: strippedPath, - }, - ), + if (!matchedRoutes || matchedRoutes.length === 0) { + return ''; + } + + for (const match of matchedRoutes) { + if (match.route.path && match.route.path !== '*') { + const path = pickPath(match); + const strippedPath = stripBasenameFromPathname(location.pathname, prefixWithSlash(match.pathnameBase)); + + return trimSlash( + trimSlash(path || '') + + prefixWithSlash( + rebuildRoutePathFromAllRoutes( + allRoutes.filter(route => route !== match.route), + { + pathname: strippedPath, + }, ), - ); - } + ), + ); } } @@ -517,7 +533,13 @@ export function createV6CompatibleWithSentryReactRouterRouting