Skip to content

fix(react): Support lazy-loaded routes and components. #15039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 23, 2025
Merged

fix(react): Support lazy-loaded routes and components. #15039

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
acf0082
fix(react): Wait for lazy-loaded pages on navigation
onurtemizkan Jan 16, 2025
1e023fb
Conditionally wait for the render depending on the router state.
onurtemizkan Jan 17, 2025
2a8888d
Support lazy-loaded descendant routes.
onurtemizkan Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions dev-packages/e2e-tests/test-applications/react-create-browser-router-lazy-routes/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions dev-packages/e2e-tests/test-applications/react-create-browser-router-lazy-routes/.npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@sentry:registry=http://127.0.0.1:4873
@sentry-internal:registry=http://127.0.0.1:4873
40 changes: 40 additions & 0 deletions ...packages/e2e-tests/test-applications/react-create-browser-router-lazy-routes/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"name": "react-create-browser-router-lazy-routes-test",
"version": "0.1.0",
"private": true,
"dependencies": {
"@sentry/react": "latest || *",
"@types/node": "^18.19.1",
"@types/react": "18.0.0",
"@types/react-dom": "18.0.0",
"react": "18.2.0",
"react-dom": "18.2.0",
"react-router-dom": "^6.4.1",
"react-scripts": "5.0.1",
"typescript": "~5.0.0"
},
"scripts": {
"build": "react-scripts build",
"start": "serve -s build",
"test": "playwright test",
"clean": "npx rimraf node_modules pnpm-lock.yaml",
"test:build": "pnpm install && pnpm build",
"test:build-canary": "pnpm install && pnpm add react@canary react-dom@canary && 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"
},
"volta": {
"extends": "../../package.json"
}
}
7 changes: 7 additions & 0 deletions ...e2e-tests/test-applications/react-create-browser-router-lazy-routes/playwright.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { getPlaywrightConfig } from '@sentry-internal/test-utils';

const config = getPlaywrightConfig({
startCommand: `pnpm start`,
});

export default config;
24 changes: 24 additions & 0 deletions ...ges/e2e-tests/test-applications/react-create-browser-router-lazy-routes/public/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<meta name="theme-color" content="#000000" />
<meta name="description" content="Web site created using create-react-app" />
<title>React App</title>
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
<!--
This HTML file is a template.
If you open it directly in the browser, you will see an empty page.

You can add webfonts, meta tags, or analytics to this file.
The build step will place the bundled scripts into the <body> tag.

To begin the development, run `npm start` or `yarn start`.
To create a production bundle, use `npm run build` or `yarn build`.
-->
</body>
</html>
5 changes: 5 additions & 0 deletions ...ages/e2e-tests/test-applications/react-create-browser-router-lazy-routes/src/globals.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
interface Window {
recordedTransactions?: string[];
capturedExceptionId?: string;
sentryReplayId?: string;
}
71 changes: 71 additions & 0 deletions ...ackages/e2e-tests/test-applications/react-create-browser-router-lazy-routes/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import * as Sentry from '@sentry/react';
import React, { Suspense, lazy } from 'react';
import ReactDOM from 'react-dom/client';
import {
RouterProvider,
createBrowserRouter,
createRoutesFromChildren,
matchRoutes,
useLocation,
useNavigationType,
} from 'react-router-dom';
import Index from './pages/Index';

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,
}),
replay,
],
// We recommend adjusting this value in production, or using tracesSampler
// for finer control
tracesSampleRate: 1.0,
release: 'e2e-test',

tunnel: 'http://localhost:3031',

// Always capture replays, so we can test this properly
replaysSessionSampleRate: 1.0,
replaysOnErrorSampleRate: 0.0,

debug: !!process.env.DEBUG,
});

const sentryCreateBrowserRouter = Sentry.wrapCreateBrowserRouterV6(createBrowserRouter);

const User = lazy(() => import('./pages/User'));

const router = sentryCreateBrowserRouter(
[
{
path: '/',
element: <Index />,
},
{
element: (
<Suspense fallback={<div>Loading...</div>}>
Copy link
Member

Choose a reason for hiding this comment

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

No strong feelings, this is also fine, but could we possibly add this to an existing e2e test app? Would save a little but of ci/processing time 😅

Copy link
Member

Choose a reason for hiding this comment

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

We could add this to react-create-browser-router :)

<User />
</Suspense>
),
path: '/user/:id',
},
],
{
// We're testing whether this option is avoided in the integration
// We expect this to be ignored
initialEntries: ['/user/1'],
},
);

const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement);

root.render(<RouterProvider router={router} />);
23 changes: 23 additions & 0 deletions ...s/e2e-tests/test-applications/react-create-browser-router-lazy-routes/src/pages/Index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 (
<>
<input
type="button"
value="Capture Exception"
id="exception-button"
onClick={() => {
throw new Error('I am an error!');
}}
/>
<Link to="/user/5" id="navigation">
navigate
</Link>
</>
);
};

export default Index;
8 changes: 8 additions & 0 deletions ...es/e2e-tests/test-applications/react-create-browser-router-lazy-routes/src/pages/User.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const User = () => {
return <p>I am a blank page :)</p>;
};

export default User;
1 change: 1 addition & 0 deletions ...2e-tests/test-applications/react-create-browser-router-lazy-routes/src/react-app-env.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/// <reference types="react-scripts" />
6 changes: 6 additions & 0 deletions ...e2e-tests/test-applications/react-create-browser-router-lazy-routes/start-event-proxy.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { startEventProxyServer } from '@sentry-internal/test-utils';

startEventProxyServer({
port: 3031,
proxyServerName: 'react-create-browser-router',
});
30 changes: 30 additions & 0 deletions .../e2e-tests/test-applications/react-create-browser-router-lazy-routes/tests/errors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { expect, test } from '@playwright/test';
import { waitForError } from '@sentry-internal/test-utils';

test('Captures exception correctly', async ({ page }) => {
const errorEventPromise = waitForError('react-create-browser-router', event => {
return !event.type && event.exception?.values?.[0]?.value === 'I am an error!';
});

await page.goto('/');

const exceptionButton = page.locator('id=exception-button');
await exceptionButton.click();

const errorEvent = await errorEventPromise;

expect(errorEvent.exception?.values).toHaveLength(1);
expect(errorEvent.exception?.values?.[0]?.value).toBe('I am an error!');

expect(errorEvent.request).toEqual({
headers: expect.any(Object),
url: 'http://localhost:3030/',
});

expect(errorEvent.transaction).toEqual('/');

expect(errorEvent.contexts?.trace).toEqual({
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
span_id: expect.stringMatching(/[a-f0-9]{16}/),
});
});
83 changes: 83 additions & 0 deletions ...ests/test-applications/react-create-browser-router-lazy-routes/tests/transactions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { expect, test } from '@playwright/test';
import { waitForTransaction } from '@sentry-internal/test-utils';

test('Captures a pageload transaction', async ({ page }) => {
const transactionEventPromise = waitForTransaction('react-create-browser-router', event => {
return event.contexts?.trace?.op === 'pageload';
});

await page.goto('/');

const transactionEvent = await transactionEventPromise;

expect(transactionEvent).toEqual(
expect.objectContaining({
transaction: '/',
type: 'transaction',
transaction_info: {
source: 'route',
},
}),
);

expect(transactionEvent.contexts?.trace).toEqual(
expect.objectContaining({
data: expect.objectContaining({
deviceMemory: expect.any(String),
effectiveConnectionType: expect.any(String),
hardwareConcurrency: expect.any(String),
'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',
}),
);
});

test('Captures a 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=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: '/user/:id',
type: 'transaction',
transaction_info: {
source: 'route',
},
}),
);

expect(transactionEvent.spans).toEqual([
expect.objectContaining({
op: 'resource.script',
origin: 'auto.resource.browser.metrics',
}),
]);
});
20 changes: 20 additions & 0 deletions ...ackages/e2e-tests/test-applications/react-create-browser-router-lazy-routes/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"]
}
28 changes: 20 additions & 8 deletions packages/react/src/reactrouterv6-compat-utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,27 @@ export function createV6CompatibleWrapCreateBrowserRouter<
}

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(() => {
Copy link
Member

@s1gr1d s1gr1d Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, my idea with requestAnimationFrame actually worked - that's nice ✨
Thanks for implementing 🙌

But as Francesco pointed out - we should keep in mind the slight overhead and maybe there's even another way to implement this 🤔 Do we know if a route is lazy?

handleNavigation({
location: state.location,
routes,
navigationType: state.historyAction,
version,
basename,
});
});
} else {
handleNavigation({
location: state.location,
routes,
navigationType: state.historyAction,
version,
basename,
});
}
}
});

Expand Down
Loading
Loading