Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(react): Add react-router-v6 integration. #5042

Merged
merged 13 commits into from
May 11, 2022

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented May 5, 2022

Tracing integration for [email protected]

This implementation will provide a HoC that wraps <Routes> which replaced <Switch> from [email protected].

This approach is similar to @wontondon's solution here.

Usage example:

import ReactDOM from "react-dom";
import {
    Routes,
    BrowserRouter,
    useLocation,
    useNavigationType,
    createRoutesFromChildren,
    matchRoutes,
} from "react-router-dom";
import * as Sentry from "@sentry/react";
import "@sentry/tracing";

Sentry.init({
    dsn: "DSN",
    integrations: [
        new BrowserTracing({
            routingInstrumentation: Sentry.reactRouterV6Instrumentation(
                React.useEffect,
                useLocation,
                useNavigationType,
                createRoutesFromChildren,
                matchRoutes,
            ),
        }),
    ],
    tracesSampleRate: 1.0,
});

const SentryRoutes = Sentry.withSentryV6(Routes)

ReactDOM.render(
    <BrowserRouter>
        <SentryRoutes>
            <Route path="/" element={<div>Home</div>} />
        </SentryRoutes>
    </BrowserRouter>,
);

Resolves: #4118

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 18.78 KB (-6.77% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 58.38 KB (-9.65% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.54 KB (-6.98% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 52.61 KB (-9.26% 🔽)
@sentry/browser - Webpack (gzipped + minified) 19.33 KB (-16.83% 🔽)
@sentry/browser - Webpack (minified) 61.47 KB (-24.77% 🔽)
@sentry/react - Webpack (gzipped + minified) 19.35 KB (-16.87% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 42.81 KB (-10.91% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 24.43 KB (-6.3% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23 KB (-6.04% 🔽)

Comment on lines 55 to 62
instrumentationOptions.useLocation = useLocation;
instrumentationOptions.useNavigationType = useNavigationType;
instrumentationOptions.matchRoutes = matchRoutes;
instrumentationOptions.createRoutesFromChildren = createRoutesFromChildren;

instrumentationOptions.customStartTransaction = customStartTransaction;
instrumentationOptions.startTransactionOnLocationChange = startTransactionOnLocationChange;
instrumentationOptions.startTransactionOnPageLoad = startTransactionOnPageLoad;
Copy link
Member

Choose a reason for hiding this comment

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

Let's lift these up into individual variables instead of being under the instrumentationOptions object. This way we save on bundle size because otherwise object keys like customStartTransaction or startTransactionOnLocationChange cannot be minified.

Suggested change
instrumentationOptions.useLocation = useLocation;
instrumentationOptions.useNavigationType = useNavigationType;
instrumentationOptions.matchRoutes = matchRoutes;
instrumentationOptions.createRoutesFromChildren = createRoutesFromChildren;
instrumentationOptions.customStartTransaction = customStartTransaction;
instrumentationOptions.startTransactionOnLocationChange = startTransactionOnLocationChange;
instrumentationOptions.startTransactionOnPageLoad = startTransactionOnPageLoad;
useLocation = useLocation;
useNavigationType = useNavigationType;
matchRoutes = matchRoutes;
createRoutesFromChildren = createRoutesFromChildren;
customStartTransaction = customStartTransaction;
startTransactionOnLocationChange = startTransactionOnLocationChange;
startTransactionOnPageLoad = startTransactionOnPageLoad;

@onurtemizkan onurtemizkan marked this pull request as ready for review May 6, 2022 13:15
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Looks good!

Can we add another test case around parameterized paths? Specifically with multi-parameter paths like orgs/:orgid/teams/:teamid

'routing.instrumentation': 'react-router-v6',
};

export function withSentryV6<P extends Record<string, any>, R extends React.FC<P>>(Routes: R): R {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this withSentryReactRouterV6Routing. It's way longer, but it makes it very clear what is happening to users (+ will all get minified in the end anyway).

startTransactionOnPageLoad = true,
startTransactionOnLocationChange = true,
): void => {
_useEffect = useEffect;
Copy link
Member

Choose a reason for hiding this comment

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

Can we start the transaction here and just update the transaction name in the useeffect call?

Also, is there a way we can listen directly onto history instead of using useEffect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to start pageload transaction on init 👍

Tried to find a simple solution to access history but it doesn't look possible without risking reliability (as they're encouraging to use useNavigate and useLocation hooks in v6, so even if we try to use global history, they may conflict?). Combining them with useEffect seems to be the most reliable, but can dig deeper if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Ok - no worries, let's keep using the hooks then!

Comment on lines 140 to 143
const transactionName = getTransactionName(routes, location, _matchRoutes);

activeTransaction = _customStartTransaction({
name: transactionName,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const transactionName = getTransactionName(routes, location, _matchRoutes);
activeTransaction = _customStartTransaction({
name: transactionName,
activeTransaction = _customStartTransaction({
name: getTransactionName(routes, location, _matchRoutes),

!_matchRoutes ||
!_customStartTransaction
) {
// Log warning?
Copy link
Member

Choose a reason for hiding this comment

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

yeah we should log something here

packages/react/test/reactrouterv6.test.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,177 @@
import { Transaction, TransactionContext } from '@sentry/types';
Copy link
Member

Choose a reason for hiding this comment

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

Can we make sure we attribute to whom this implementation was inspired/based on?

@AbhiPrasad AbhiPrasad changed the title feat(tracing): Add react-router-v6 integration. feat(react): Add react-router-v6 integration. May 11, 2022
@AbhiPrasad
Copy link
Member

We will also need a docs PR for this!

@AbhiPrasad AbhiPrasad merged commit cd71864 into 7.x May 11, 2022
@AbhiPrasad AbhiPrasad deleted the onur/react-router-v6-integration branch May 11, 2022 15:33
@AbhiPrasad AbhiPrasad added this to the 7.0.0 milestone May 12, 2022
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
Tracing integration for [`[email protected]`](https://reactrouter.com/docs/en/v6)

This implementation will provide a HoC that wraps [`<Routes>`](https://reactrouter.com/docs/en/v6/api#routes-and-route) which replaced `<Switch>` from `[email protected]`.
@FilipaBarroso
Copy link

Is there a way to use this integration with useRoutes, or any plans to support it in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants