From b49c1cc05e79f3854b642066ef1176ef9eb3f8d0 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 23 Jan 2025 08:43:22 +0000 Subject: [PATCH] fix(react): Support lazy-loaded routes and components. (#15039) Fixes: https://github.com/getsentry/sentry-javascript/issues/15027 This PR adds support for lazily loaded components and routes inside `Suspend` on react-router pageloads / navigations. --- .../react-create-browser-router/src/index.tsx | 11 +- .../src/pages/Index.tsx | 3 + .../src/pages/LazyLoadedInnerRoute.tsx | 14 +++ .../src/pages/LazyLoadedUser.tsx | 23 ++++ .../tests/transactions.test.ts | 102 ++++++++++++++++++ .../react/src/reactrouterv6-compat-utils.tsx | 62 ++++++++--- packages/react/src/types.ts | 11 +- 7 files changed, 205 insertions(+), 21 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedInnerRoute.tsx create mode 100644 dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedUser.tsx diff --git a/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/index.tsx index 88f8cfa502ec..c7ad16eebcf7 100644 --- a/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/index.tsx @@ -1,5 +1,5 @@ import * as Sentry from '@sentry/react'; -import React from 'react'; +import React, { lazy, Suspense } from 'react'; import ReactDOM from 'react-dom/client'; import { RouterProvider, @@ -42,6 +42,7 @@ Sentry.init({ }); const sentryCreateBrowserRouter = Sentry.wrapCreateBrowserRouterV6(createBrowserRouter); +const LazyLoadedUser = lazy(() => import('./pages/LazyLoadedUser')); const router = sentryCreateBrowserRouter( [ @@ -49,6 +50,14 @@ const router = sentryCreateBrowserRouter( path: '/', element: , }, + { + path: '/lazy-loaded-user/*', + element: ( + Loading...}> + + + ), + }, { path: '/user/:id', element: , diff --git a/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/Index.tsx b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/Index.tsx index d6b71a1d1279..12bfb12ec3a9 100644 --- a/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/Index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/Index.tsx @@ -16,6 +16,9 @@ const Index = () => { navigate + + lazy navigate + ); }; diff --git a/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedInnerRoute.tsx b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedInnerRoute.tsx new file mode 100644 index 000000000000..1410df69124b --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedInnerRoute.tsx @@ -0,0 +1,14 @@ +import * as Sentry from '@sentry/react'; +// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX +import * as React from 'react'; +import { Route, Routes } from 'react-router-dom'; + +const SentryRoutes = Sentry.withSentryReactRouterV6Routing(Routes); + +const InnerRoute = () => ( + + I am a lazy loaded user

} /> + +); + +export default InnerRoute; diff --git a/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedUser.tsx b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedUser.tsx new file mode 100644 index 000000000000..636f99d9c8cb --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/react-create-browser-router/src/pages/LazyLoadedUser.tsx @@ -0,0 +1,23 @@ +import * as Sentry from '@sentry/react'; +import * as React from 'react'; +import { Route, Routes } from 'react-router-dom'; + +const SentryRoutes = Sentry.withSentryReactRouterV6Routing(Routes); +const InnerRoute = React.lazy(() => import('./LazyLoadedInnerRoute')); + +const LazyLoadedUser = () => { + return ( + + Loading...

}> + + + } + /> + + ); +}; + +export default LazyLoadedUser; diff --git a/dev-packages/e2e-tests/test-applications/react-create-browser-router/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-create-browser-router/tests/transactions.test.ts index 5ecd098daf94..c35d731915d6 100644 --- a/dev-packages/e2e-tests/test-applications/react-create-browser-router/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-create-browser-router/tests/transactions.test.ts @@ -76,3 +76,105 @@ test('Captures a navigation transaction', async ({ page }) => { expect(transactionEvent.spans).toEqual([]); }); + +test('Captures a lazy pageload transaction', async ({ page }) => { + const transactionEventPromise = waitForTransaction('react-create-browser-router', event => { + return event.contexts?.trace?.op === 'pageload'; + }); + + await page.goto('/lazy-loaded-user/5/foo'); + + const transactionEvent = await transactionEventPromise; + expect(transactionEvent.contexts?.trace).toEqual({ + data: expect.objectContaining({ + 'sentry.idle_span_finish_reason': 'idleTimeout', + 'sentry.op': 'pageload', + 'sentry.origin': 'auto.pageload.react.reactrouter_v6', + 'sentry.sample_rate': 1, + 'sentry.source': 'route', + }), + op: 'pageload', + span_id: expect.stringMatching(/[a-f0-9]{16}/), + trace_id: expect.stringMatching(/[a-f0-9]{32}/), + origin: 'auto.pageload.react.reactrouter_v6', + }); + + expect(transactionEvent).toEqual( + expect.objectContaining({ + transaction: '/lazy-loaded-user/:id/:innerId', + type: 'transaction', + transaction_info: { + source: 'route', + }, + }), + ); + + expect(await page.innerText('id=content')).toContain('I am a lazy loaded user'); + + expect(transactionEvent.spans).toEqual( + expect.arrayContaining([ + // This one is the outer lazy route + expect.objectContaining({ + op: 'resource.script', + origin: 'auto.resource.browser.metrics', + }), + // This one is the inner lazy route + expect.objectContaining({ + op: 'resource.script', + origin: 'auto.resource.browser.metrics', + }), + ]), + ); +}); + +test('Captures a lazy navigation transaction', async ({ page }) => { + const transactionEventPromise = waitForTransaction('react-create-browser-router', event => { + return event.contexts?.trace?.op === 'navigation'; + }); + + await page.goto('/'); + const linkElement = page.locator('id=lazy-navigation'); + await linkElement.click(); + + const transactionEvent = await transactionEventPromise; + expect(transactionEvent.contexts?.trace).toEqual({ + data: expect.objectContaining({ + 'sentry.idle_span_finish_reason': 'idleTimeout', + 'sentry.op': 'navigation', + 'sentry.origin': 'auto.navigation.react.reactrouter_v6', + 'sentry.sample_rate': 1, + 'sentry.source': 'route', + }), + op: 'navigation', + span_id: expect.stringMatching(/[a-f0-9]{16}/), + trace_id: expect.stringMatching(/[a-f0-9]{32}/), + origin: 'auto.navigation.react.reactrouter_v6', + }); + + expect(transactionEvent).toEqual( + expect.objectContaining({ + transaction: '/lazy-loaded-user/:id/:innerId', + type: 'transaction', + transaction_info: { + source: 'route', + }, + }), + ); + + expect(await page.innerText('id=content')).toContain('I am a lazy loaded user'); + + expect(transactionEvent.spans).toEqual( + expect.arrayContaining([ + // This one is the outer lazy route + expect.objectContaining({ + op: 'resource.script', + origin: 'auto.resource.browser.metrics', + }), + // This one is the inner lazy route + expect.objectContaining({ + op: 'resource.script', + origin: 'auto.resource.browser.metrics', + }), + ]), + ); +}); diff --git a/packages/react/src/reactrouterv6-compat-utils.tsx b/packages/react/src/reactrouterv6-compat-utils.tsx index 9ca96a03b0de..db65c32cee99 100644 --- a/packages/react/src/reactrouterv6-compat-utils.tsx +++ b/packages/react/src/reactrouterv6-compat-utils.tsx @@ -61,6 +61,9 @@ export interface ReactRouterOptions { type V6CompatibleVersion = '6' | '7'; +// Keeping as a global variable for cross-usage in multiple functions +const allRoutes = new Set(); + /** * Creates a wrapCreateBrowserRouter function that can be used with all React Router v6 compatible versions. */ @@ -81,6 +84,10 @@ export function createV6CompatibleWrapCreateBrowserRouter< } return function (routes: RouteObject[], opts?: Record & { basename?: string }): TRouter { + routes.forEach(route => { + allRoutes.add(route); + }); + const router = createRouterFunction(routes, opts); const basename = opts?.basename; @@ -90,19 +97,40 @@ export function createV6CompatibleWrapCreateBrowserRouter< // This is the earliest convenient time to update the transaction name. // Callbacks to `router.subscribe` are not called for the initial load. if (router.state.historyAction === 'POP' && activeRootSpan) { - updatePageloadTransaction(activeRootSpan, router.state.location, routes, undefined, basename); + updatePageloadTransaction( + activeRootSpan, + router.state.location, + routes, + undefined, + basename, + Array.from(allRoutes), + ); } router.subscribe((state: RouterState) => { - const location = state.location; if (state.historyAction === 'PUSH' || state.historyAction === 'POP') { - handleNavigation({ - location, - routes, - navigationType: state.historyAction, - version, - basename, - }); + // Wait for the next render if loading an unsettled route + if (state.navigation.state !== 'idle') { + requestAnimationFrame(() => { + handleNavigation({ + location: state.location, + routes, + navigationType: state.historyAction, + version, + basename, + allRoutes: Array.from(allRoutes), + }); + }); + } else { + handleNavigation({ + location: state.location, + routes, + navigationType: state.historyAction, + version, + basename, + allRoutes: Array.from(allRoutes), + }); + } } }); @@ -137,6 +165,10 @@ export function createV6CompatibleWrapCreateMemoryRouter< initialIndex?: number; }, ): TRouter { + routes.forEach(route => { + allRoutes.add(route); + }); + const router = createRouterFunction(routes, opts); const basename = opts?.basename; @@ -162,7 +194,7 @@ export function createV6CompatibleWrapCreateMemoryRouter< : router.state.location; if (router.state.historyAction === 'POP' && activeRootSpan) { - updatePageloadTransaction(activeRootSpan, location, routes, undefined, basename); + updatePageloadTransaction(activeRootSpan, location, routes, undefined, basename, Array.from(allRoutes)); } router.subscribe((state: RouterState) => { @@ -174,6 +206,7 @@ export function createV6CompatibleWrapCreateMemoryRouter< navigationType: state.historyAction, version, basename, + allRoutes: Array.from(allRoutes), }); } }); @@ -248,8 +281,6 @@ export function createV6CompatibleWrapUseRoutes(origUseRoutes: UseRoutes, versio return origUseRoutes; } - const allRoutes: Set = new Set(); - const SentryRoutes: React.FC<{ children?: React.ReactNode; routes: RouteObject[]; @@ -319,7 +350,6 @@ export function handleNavigation(opts: { 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(); @@ -553,7 +583,7 @@ function updatePageloadTransaction( ): void { const branches = Array.isArray(matches) ? matches - : (_matchRoutes(routes, location, basename) as unknown as RouteMatch[]); + : (_matchRoutes(allRoutes || routes, location, basename) as unknown as RouteMatch[]); if (branches) { let name, @@ -569,7 +599,7 @@ function updatePageloadTransaction( [name, source] = getNormalizedName(routes, location, branches, basename); } - getCurrentScope().setTransactionName(name); + getCurrentScope().setTransactionName(name || '/'); if (activeRootSpan) { activeRootSpan.updateName(name); @@ -592,8 +622,6 @@ export function createV6CompatibleWithSentryReactRouterRouting

= new Set(); - const SentryRoutes: React.FC

= (props: P) => { const isMountRenderPass = React.useRef(true); diff --git a/packages/react/src/types.ts b/packages/react/src/types.ts index 1a40ec4fce91..b29a2dbd1cad 100644 --- a/packages/react/src/types.ts +++ b/packages/react/src/types.ts @@ -182,10 +182,14 @@ export interface RouterInit { hydrationData?: HydrationState; } +export type NavigationState = { + state: 'idle' | 'loading' | 'submitting'; +}; + export type NavigationStates = { - Idle: any; - Loading: any; - Submitting: any; + Idle: NavigationState; + Loading: NavigationState; + Submitting: NavigationState; }; export type Navigation = NavigationStates[keyof NavigationStates]; @@ -202,6 +206,7 @@ export declare enum HistoryAction { export interface RouterState { historyAction: Action | HistoryAction | any; location: Location; + navigation: Navigation; } export interface Router { state: TState;