Skip to content
New issue

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

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

Jump to bottom

ref(react): Add source to react-router-v3 #5377

Merged
merged 1 commit into from
Jul 7, 2022
+101 −22
Merged

ref(react): Add source to react-router-v3 #5377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 27 additions & 14 deletions packages/react/src/reactrouterv3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Primitive, Transaction, TransactionContext } from '@sentry/types';
import { Primitive, Transaction, TransactionContext, TransactionSource } from '@sentry/types';
import { getGlobalObject } from '@sentry/utils';

import { Location, ReactRouterInstrumentation } from './types';
Expand All @@ -19,6 +19,8 @@ export type Match = (
cb: (error?: Error, _redirectLocation?: Location, renderProps?: { routes?: Route[] }) => void,
) => void;

type ReactRouterV3TransactionSource = Extract<TransactionSource, 'url' | 'route'>;

const global = getGlobalObject<Window>();

/**
Expand All @@ -44,16 +46,24 @@ export function reactRouterV3Instrumentation(

// Have to use global.location because history.location might not be defined.
if (startTransactionOnPageLoad && global && global.location) {
normalizeTransactionName(routes, global.location as unknown as Location, match, (localName: string) => {
prevName = localName;
activeTransaction = startTransaction({
name: prevName,
op: 'pageload',
tags: {
'routing.instrumentation': 'react-router-v3',
},
});
});
normalizeTransactionName(
routes,
global.location as unknown as Location,
match,
(localName: string, source: ReactRouterV3TransactionSource = 'url') => {
prevName = localName;
activeTransaction = startTransaction({
name: prevName,
op: 'pageload',
tags: {
'routing.instrumentation': 'react-router-v3',
},
metadata: {
source,
},
});
},
);
}

if (startTransactionOnLocationChange && history.listen) {
Expand All @@ -68,12 +78,15 @@ export function reactRouterV3Instrumentation(
if (prevName) {
tags.from = prevName;
}
normalizeTransactionName(routes, location, match, (localName: string) => {
normalizeTransactionName(routes, location, match, (localName: string, source: TransactionSource = 'url') => {
prevName = localName;
activeTransaction = startTransaction({
name: prevName,
op: 'navigation',
tags,
metadata: {
source,
},
});
});
}
Expand All @@ -89,7 +102,7 @@ function normalizeTransactionName(
appRoutes: Route[],
location: Location,
match: Match,
callback: (pathname: string) => void,
callback: (pathname: string, source?: ReactRouterV3TransactionSource) => void,
): void {
let name = location.pathname;
match(
Expand All @@ -108,7 +121,7 @@ function normalizeTransactionName(
}

name = routePath;
return callback(name);
return callback(name, 'route');
},
);
}
Expand Down
82 changes: 74 additions & 8 deletions packages/react/test/reactrouterv3.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ declare module 'react-router-3' {
export const createRoutes: (routes: any) => RouteType[];
}

function createMockStartTransaction(opts: { finish?: jest.FunctionLike; setMetadata?: jest.FunctionLike } = {}) {
const { finish = jest.fn(), setMetadata = jest.fn() } = opts;
return jest.fn().mockReturnValue({
finish,
setMetadata,
});
}

describe('React Router V3', () => {
const routes = (
<Route path="/" component={({ children }: { children: JSX.Element }) => <div>{children}</div>}>
Expand All @@ -37,24 +45,27 @@ describe('React Router V3', () => {
const instrumentation = reactRouterV3Instrumentation(history, instrumentationRoutes, match);

it('starts a pageload transaction when instrumentation is started', () => {
const mockStartTransaction = jest.fn();
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction);
expect(mockStartTransaction).toHaveBeenCalledTimes(1);
expect(mockStartTransaction).toHaveBeenLastCalledWith({
name: '/',
op: 'pageload',
tags: { 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'route',
},
});
});

it('does not start pageload transaction if option is false', () => {
const mockStartTransaction = jest.fn();
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction, false);
expect(mockStartTransaction).toHaveBeenCalledTimes(0);
});

it('starts a navigation transaction', () => {
const mockStartTransaction = jest.fn();
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction);
render(<Router history={history}>{routes}</Router>);

Expand All @@ -66,6 +77,9 @@ describe('React Router V3', () => {
name: '/about',
op: 'navigation',
tags: { from: '/', 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'route',
},
});

act(() => {
Expand All @@ -76,18 +90,21 @@ describe('React Router V3', () => {
name: '/features',
op: 'navigation',
tags: { from: '/about', 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'route',
},
});
});

it('does not start a transaction if option is false', () => {
const mockStartTransaction = jest.fn();
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction, true, false);
render(<Router history={history}>{routes}</Router>);
expect(mockStartTransaction).toHaveBeenCalledTimes(1);
});

it('only starts a navigation transaction on push', () => {
const mockStartTransaction = jest.fn();
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction);
render(<Router history={history}>{routes}</Router>);

Expand All @@ -99,7 +116,7 @@ describe('React Router V3', () => {

it('finishes a transaction on navigation', () => {
const mockFinish = jest.fn();
const mockStartTransaction = jest.fn().mockReturnValue({ finish: mockFinish });
const mockStartTransaction = createMockStartTransaction({ finish: mockFinish });
instrumentation(mockStartTransaction);
render(<Router history={history}>{routes}</Router>);
expect(mockStartTransaction).toHaveBeenCalledTimes(1);
Expand All @@ -112,7 +129,7 @@ describe('React Router V3', () => {
});

it('normalizes transaction names', () => {
const mockStartTransaction = jest.fn();
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction);
const { container } = render(<Router history={history}>{routes}</Router>);

Expand All @@ -126,11 +143,14 @@ describe('React Router V3', () => {
name: '/users/:userid',
op: 'navigation',
tags: { from: '/', 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'route',
},
});
});

it('normalizes nested transaction names', () => {
const mockStartTransaction = jest.fn();
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction);
const { container } = render(<Router history={history}>{routes}</Router>);

Expand All @@ -144,6 +164,9 @@ describe('React Router V3', () => {
name: '/organizations/:orgid/v1/:teamid',
op: 'navigation',
tags: { from: '/', 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'route',
},
});

act(() => {
Expand All @@ -156,6 +179,49 @@ describe('React Router V3', () => {
name: '/organizations/:orgid',
op: 'navigation',
tags: { from: '/organizations/:orgid/v1/:teamid', 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'route',
},
});
});

it('sets metadata to url if on an unknown route', () => {
const mockStartTransaction = createMockStartTransaction();
instrumentation(mockStartTransaction);
render(<Router history={history}>{routes}</Router>);

act(() => {
history.push('/organizations/1234/some/other/route');
});

expect(mockStartTransaction).toHaveBeenCalledTimes(2);
expect(mockStartTransaction).toHaveBeenLastCalledWith({
name: '/organizations/1234/some/other/route',
op: 'navigation',
tags: { from: '/', 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'url',
},
});
});

it('sets metadata to url if no routes are provided', () => {
const fakeRoutes = <div>hello</div>;
const mockStartTransaction = createMockStartTransaction();
const mockInstrumentation = reactRouterV3Instrumentation(history, createRoutes(fakeRoutes), match);
mockInstrumentation(mockStartTransaction);
// We render here with `routes` instead of `fakeRoutes` from above to validate the case
// where users provided the instrumentation with a bad set of routes.
render(<Router history={history}>{routes}</Router>);

expect(mockStartTransaction).toHaveBeenCalledTimes(1);
expect(mockStartTransaction).toHaveBeenLastCalledWith({
name: '/',
op: 'pageload',
tags: { 'routing.instrumentation': 'react-router-v3' },
metadata: {
source: 'url',
},
});
});
});