-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support React Router v6 #4118
Comments
Awesome work man, the approach looks good from an initial glance. If you want to, feel free to throw up a PR to add this our |
@CharlesStover anyway to use the integration when calling Sentry.init before render? |
@AbhiPrasad just for notes sake, in case y'all tackle this before I do:
@amccloud Unfortunately, as far as I'm aware, you cannot integrate with React Router v6 before render. Since React Router v6 does not let you pass the |
As a further progress report, I have:
The code base should therefore hypothetically be ready to merge into EDIT: What does "credit" look like for a contribution like this? Is there concern with simply using |
I’m planning to take a look at adding this next week, but feel free to jump on it! As per https://develop.sentry.dev/sdk/philosophy/#dependencies-cost, we shy away from external deps as much as possible. This is also useful from a compliance and security perspective for our bigger users. This means we probably will port over the instrumentation you built (and adjust it for the sdk constraints + add tests). Credit here means we will leave a code comment to your GH repo + your name. We did something similar with Python and ASGI: https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/asgi.py#L4 |
Awesome and makes sense. It's already 100% tested. With that example for Python, I should be able to PR this change, assuming I can integrate with your test runner intuitively. |
Hey @CharlesStover, how does your hook deal with parameterized URLs? (Like We have to make sure we report the parameterized URLs, not the actual one, so that transactions can be grouped properly. |
Thanks for the follow-up. My hook's implementation is a copy of the |
Yeah I wonder if there’s an easy way for us to just get the paramterized URL without recomputing the match or requiring excess things in user config. I’m going to try something out based on your hook, but if not will also try opening an issue with the react router folks to see if they have any ideas. Appreciate your help with this @CharlesStover! |
Alright made a discussion on remix-run/react-router#8327, let's see what happens. |
Any progress on this? |
Hey @marklai1998, currently we are waiting for a response from the remix folks (in the discussion linked above), to see if they can help us out. If that doesn't go anywhere, I guess we'll just have to figure it out ourselves - so no ETA atm. |
FWIW, the package mentioned in the OP can unblock anyone in the meantime. 😅 @marklai1998 |
FYI, react-router just merged a |
Unfortunately no, I do not support that feature. There is an open issue with React Router to expose the unparsed path. |
Since we already have a |
|
Typescript is not happy when passing History v5 to |
I was wrong, React Router v5 depends on history v4. I'm guessing they are waiting on input from the React Router team (remix-run/react-router#8327), before integrating a v6 instrumentation, but they are hard to reach. Maybe someone should ping them? |
Can i intergrate react-router without integration when init??? like this: function APP () {
const location = useLocation();
useEffect(() => {
getCurrentHub().withScope(() => {
// do something
})
}, [location])
} Then we can init sentry before render. |
FWIW, I created a copy of the V5 history object and monkey patched the listener as that appears to be the only incompatibility (according to typescript defs, I didn't go deeper into code to see if that's the only difference 🤷 )
import {createHashHistory} from "history";
import {Action, Location} from "@sentry/react/dist/types";
export const hashHistory = createHashHistory({ window })
export const hashHistoryV4Compat = {
...hashHistory,
listen: (cb: (location: Location, action: Action) => void) => {
return hashHistory.listen((update) => {
cb(update.location, update.action);
})
}
}
import {hashHistoryV4Compat} from "./history";
import * as Sentry from '@sentry/react'
Sentry.init({
integrations: [
new BrowserTracing({
routingInstrumentation: Sentry.reactRouterV5Instrumentation(hashHistoryV4Compat),
}),
]
})
import "./sentry";
import {unstable_HistoryRouter as Router} from 'react-router-dom'
import {hashHistory} from "./history";
....
<Router history={hashHistory}>
.... From my preliminary tests, it looks like it works just fine I'm sure that something is probably broken behind the scenes so use this at your own risk 😅, but my Sentry panel gets populated correctly In case you're wondering why |
@legraphista What did you do about using |
When I use @legraphista's tactic with the react-router-config routes as second parameter (as described in option one of the react router v5 integration guide), like so: |
function useSentry() {
const matches = useMatches();
useEffect(
() => {
Sentry.configureScope((scope) => {
scope.setTransactionName(matches[matches.length - 1].id);
});
},
matches.map((m) => m.id)
);
} Toying around with a POC on Remix and this definitely gives a usable transaction name. I'm not sure theres an event easily hookable, but we could bind this on demand in a few places assuming we can access context. |
I took a different approach to workaround this issue. I like that it uses the current react router v6 public API and supports the parameterized paths. I used inspiration from some of the components like I have published the source of Hope it helps. If it is really useful I can try and get it published. |
this is shipped in betas for V7! 🚀 we will close this request and would love to get any feedback on the usage! |
v7 has been officially released! Please see our full set of docs for using this integration: https://docs.sentry.io/platforms/javascript/guides/react/configuration/integrations/react-router/#react-router-v6 |
Is there a way to use this integration with |
Thanks @FilipaBarroso, just opened an issue for |
The correct way to integrate React Router with SentryRight place for create HOC SentryRoutesCreate HOC SentryRoutes out of component body. If you create inside the component and there are react-router-dom`s hooks especially (useLocation and useNavigate) then after build the project Routers state is cleaned up each time the route changes. That will cause all states to be reset in case of nested routes. Correct implementation
|
I'm wondering if somebody can help with this. I've just followed the directions at Sentry docs to set up Sentry with React and React Router and it's not working. I'm using React v18+ and Sentry v7+ and I only have TypeError ?(bundle) The stack trace points the error to This is the React entry point code I have: import ReactDOM from 'react-dom/client';
import * as Sentry from '@sentry/react';
import { BrowserTracing } from '@sentry/tracing';
import {
RouterProvider,
createBrowserRouter,
createRoutesFromChildren,
matchRoutes,
useLocation,
useNavigationType,
} from 'react-router-dom';
import './assets/styles/global.css';
import reportWebVitals from './reportWebVitals';
import Home from './pages/home';
import Error from './pages/error';
import MyAppointments from './pages/my-appointments';
import Meetings from './pages/meetings';
import CancelAppointment from './pages/cancel-appointment';
import { useEffect } from 'react';
// basic sentry setup without react-router
// if (process.env.REACT_APP_SENTRY_DSN) {
// Sentry.init({
// dsn: process.env.REACT_APP_SENTRY_DSN,
// integrations: [new BrowserTracing()],
// environment: process.env.NODE_ENV,
// tracesSampleRate: parseFloat(process.env.REACT_APP_SENTRY_TRACES_SAMPLE_RATE || '0.2'),
// release: process.env.REACT_APP_SENTRY_RELEASE,
// });
// }
// sentry setup with react-router instrumentation
if (process.env.REACT_APP_SENTRY_DSN) {
Sentry.init({
dsn: process.env.REACT_APP_SENTRY_DSN,
integrations: [
new BrowserTracing({
routingInstrumentation: Sentry.reactRouterV6Instrumentation(
useEffect,
useLocation,
useNavigationType,
createRoutesFromChildren,
matchRoutes,
),
}),
],
tracesSampleRate: parseFloat(process.env.REACT_APP_SENTRY_TRACES_SAMPLE_RATE || '0.2'),
environment: process.env.NODE_ENV,
release: process.env.REACT_APP_SENTRY_RELEASE,
});
}
const sentryCreateBrowserRouter = Sentry.wrapCreateBrowserRouter(createBrowserRouter);
const router = sentryCreateBrowserRouter([
{
path: '/',
element: <Home />,
errorElement: <Error />,
},
{
path: '/my_appointments',
element: <MyAppointments />,
errorElement: <Error />,
},
{
path: '/meetings',
element: <Meetings />,
errorElement: <Error />,
},
{
path: '/cancel_appointment',
element: <CancelAppointment />,
errorElement: <Error />,
},
]);
ReactDOM.createRoot(document.getElementById('root') as HTMLElement).render(
<RouterProvider router={router} />,
);
// If you want to start measuring performance in your app, pass a function
// to log results (for example: reportWebVitals(console.log))
// or send to an analytics endpoint. Learn more: https://bit.ly/CRA-vitals
reportWebVitals(); and these are my dependencies: "dependencies": {
"@sentry/react": "^7.34.0",
"@sentry/tracing": "^7.35.0",
"axios": "^1.2.3",
"dayjs": "^1.11.7",
"react": "^18.2.0",
"react-countdown": "^2.3.5",
"react-dom": "^18.2.0",
"react-router-dom": "6",
"react-scripts": "5.0.1",
"simple-crypto-js": "^3.0.1",
"typescript": "^4.9.4",
"web-vitals": "^2.1.0"
}, I add an exception for testing purpose at |
The
reactRouterV#Instrumentation
is no longer supported in React Router v6. Unlike previous versions of React Router, v6 does not expose thehistory
object.I've attempted to solve this personally, so as to unblock myself in my projects:
react-router-v6-instrumentation
on NPM.Example usage:
Source code on GitHub
I hope this ticket inspires the maintainers to implement (credit? 😆) or unblocks others who are searching for solutions to this issue.
The text was updated successfully, but these errors were encountered: