Skip to content

beforeNavigate isn't called for ReactRouterV6 #5598

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

Closed
3 tasks done
yarinsa opened this issue Aug 17, 2022 · 15 comments
Closed
3 tasks done

beforeNavigate isn't called for ReactRouterV6 #5598

yarinsa opened this issue Aug 17, 2022 · 15 comments
Assignees
Labels
Meta: Help Wanted Package: react Issues related to the Sentry React SDK

Comments

@yarinsa
Copy link

yarinsa commented Aug 17, 2022

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/react

SDK Version

7.11.0

Framework Version

React 17.0.2

Link to Sentry event

No response

Steps to Reproduce

  1. Setup sentry instrumentation with ReactRouterV6 according to the docs
  2. Implement some dummy function for "beforeNavigate" on BrowserTracing options
    integrations: [
      new BrowserTracing({
        routingInstrumentation: Sentry.reactRouterV6Instrumentation(
          useEffect,
          useLocation,
          useNavigationType,
          createRoutesFromChildren,
          matchRoutes,
        ),
        beforeNavigate: (ctx) => {
          console.log(ctx);
          return ctx;
        },
      }),
    ],
  1. Go into the browser and navigate across different routes

Expected Result

console would print the ctx of each navigation

Actual Result

Except pageload - initial navigation , nothing happens.

Also tried to blend into "matchRoutes" function using hoisting.
Also tried to capture the event in "beforeSend"
Seems like the events doesn't go trough there.
Trying to enrich the events with the route context.

@AbhiPrasad
Copy link
Member

Hey, thanks for writing in. Could you enable debug mode by supplying debug: true as an option to the SDK init? What logs out when you turn on debug mode?

@yarinsa
Copy link
Author

yarinsa commented Aug 18, 2022

Sentry Logger [log]: [Tracing] Starting 'http.client' span on transaction '/setup' (b153127e151021d2).
logger.ts:77 Sentry Logger [log]: [Tracing] Finishing 'http.client' span on transaction '/setup' (b153127e151021d2).
instrument.ts:123 IdleTransaction {traceId: '6ec990e72481440781756c2284ce6aab', spanId: 'b153127e151021d2', startTimestamp: 1660815344.4526, tags: {…}, data: {…}, …}activities: {}data: {}endTimestamp: 1660815345.4535metadata: {source: 'url', baggage: Array(3), spanMetadata: {…}, transactionSampling: {…}}op: "pageload"sampled: falsespanId: "b153127e151021d2"startTimestamp: 1660815344.4526tags: {routing.instrumentation: 'react-router-v6'}traceId: "6ec990e72481440781756c2284ce6aab"transaction: IdleTransaction {traceId: '6ec990e72481440781756c2284ce6aab', spanId: 'b153127e151021d2', startTimestamp: 1660815344.4526, tags: {…}, data: {…}, …}_beforeFinishCallbacks: [ƒ]_finalTimeout: 30000_finished: true_heartbeatCounter: 0_hub: Hub {_version: 4, _stack: Array(1)}_idleHub: Hub {_version: 4, _stack: Array(1)}_idleTimeout: 1000_idleTimeoutID: 3_measurements: {}_name: "/setup"_onScope: true_trimEnd: truename: (...)[[Prototype]]: Transaction
logger.ts:77 Sentry Logger [log]: [Tracing] No active IdleTransaction
logger.ts:77 Sentry Logger [log]: [Tracing] Discarding transaction because its trace was not chosen to be sampled.
logger.ts:77 Sentry Logger [log]: Adding outcome: "sample_rate:transaction"
logger.ts:77 Sentry Logger [log]: [Tracing] Finishing 'http.client' span on transaction '/setup' (b153127e151021d2).
instrument.ts:123 undefined
logger.ts:77 Sentry Logger [log]: [Tracing] Finishing 'http.client' span on transaction '/setup' (b153127e151021d2).

That's on page load, which is also the only case beforeNavigate is getting called. When switching routes nothing goes to the console

Screen.Recording.2022-08-18.at.12.35.04.mov

@AbhiPrasad
Copy link
Member

Thank you for the video!

This is the logic for how navigations are generated:

_useEffect(() => {
if (isBaseLocation) {
if (activeTransaction) {
activeTransaction.finish();
}
return;
}
if (_startTransactionOnLocationChange && (navigationType === 'PUSH' || navigationType === 'POP')) {
if (activeTransaction) {
activeTransaction.finish();
}
const [name, source] = getNormalizedName(routes, location, _matchRoutes);
activeTransaction = _customStartTransaction({
name,
op: 'navigation',
tags: SENTRY_TAGS,
metadata: {
source,
},
});
}
}, [props.children, location, navigationType, isBaseLocation]);

So there's a couple things here:
a) the useEffect is not being triggered
b) the navigationType is not PUSH or POP.

Perhaps we have to adjust the dependency array of the useEffect here.

@yarinsa
Copy link
Author

yarinsa commented Sep 4, 2022

@AbhiPrasad Thank you!
Is this on your roadmap? When can we expect it to be resolved?

@AbhiPrasad
Copy link
Member

Could you check what is talked about in #5555 could resolve your issue?

@yarinsa
Copy link
Author

yarinsa commented Sep 5, 2022

This is our entry file. We have a "bootstrap" function and we are running Sentry first like it says in the docs

Screen Shot 2022-09-05 at 14 56 04

@ymatusevich
Copy link

We stuck with the same issue. Hope the solution or fix will be found soon.

@yarinsa
Copy link
Author

yarinsa commented Sep 28, 2022

@AbhiPrasad If I understand correctly the we should "Start Transaction" for each route navigation (similar to page load).
I was trying to implement something similar with my own code, but it's seems like the scope that I am setting getting lost in the global Sentry object.
I was trying to do something like:

export const handleRouteChange = (routes, location) => {
 // Close existing transaction if exists
  const transaction = Sentry.getCurrentHub().getScope().getTransaction();
  transaction?.finish();

  const route = matchRoutes(routes, location.pathname)?.[0];

  const nextTransaction = Sentry.startTransaction({
    name: route.pathname,
  });

  Sentry.getCurrentHub().withScope((scope) => {
    scope.setSpan(nextTransaction);
    scope.setExtra('route', route); // Used to identify my scope for debugging
  });
};

Then in other place on my app I did something like

    const existingTransaction = Sentry.getCurrentHub().getScope()?.getTransaction();

    // Create a transaction if one does not exist in order to work around
    // https://github.com/getsentry/sentry-javascript/issues/3169
    // https://github.com/getsentry/sentry-javascript/issues/4072
    const transaction =
      existingTransaction ??
      Sentry.startTransaction({
        name: `API Request(${config?.data?.serviceId}): ${config?.data?.endpoint ?? config.url}`,
      });
      
    Sentry.getCurrentHub().configureScope((scope) => scope.setSpan(transaction));

That's in order to attach the network error to the transaction. But existingTransaction is undefined.

I ended up creating a "single" transaction for every network endpoint& request which misses the whole idea of transactions., which feels strongly wrong. When can we expect a solution for something so basic?

@AbhiPrasad
Copy link
Member

If I understand correctly the we should "Start Transaction" for each route navigation

You shouldn't need to do this, we should be automatically creating these transactions. That's the bug that needs to be fixed. I do have some bandwidth to look at this - are you able to share some kind of repro of this problem so I can take a look?

@yarinsa
Copy link
Author

yarinsa commented Sep 28, 2022

Mmmm , it's pretty basic. Start create react app, add the react-router integration and have debug true, in the sentry init. I am willing to go on a call,

@AbhiPrasad
Copy link
Member

Ah ok, was able to reproduce with https://stackblitz.com/edit/github-7tjxzc-gabqis?file=src/App.tsx. Will take a look!

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Sep 29, 2022

Ah ok - seems like I fell into the same thing that was addressed here: #5555 (comment)

See our docs update: https://github.com/getsentry/sentry-docs/pull/5505/files

You have to make sure that Sentry.init is being called before you wrap your routes, otherwise things won't work properly.

I updated the example to move the logic into App.tsx, and everything works properly. This API doesn't feel ideal to me though, so I'm gonna try and update it. https://stackblitz.com/edit/github-7tjxzc-gabqis

You can confirm this by turning on debug mode, and searching for the string reactRouterV6Instrumentation was unable to wrap Routes because of one or more missing parameters.

@yarinsa
Copy link
Author

yarinsa commented Oct 3, 2022

@AbhiPrasad I red trough it and even had a comment in my code. My problem was that I had file called 'sentry.tsx' which contains:

export const initSentry =() => {
Sentry.init()
}

export const SentryRoutes = Sentry.withSentryReactRouterV6Routing(Routes);

The init is called in a function so the wrapping of HOC is actually happening first.

I highly suggest to create some promise based logic for this

@AbhiPrasad
Copy link
Member

I highly suggest to create some promise based logic for this

We might just eliminate this problem entirely by supporting the new APIs in 6.4 - #5872

@yarinsa
Copy link
Author

yarinsa commented Oct 12, 2022

Seems like it was solved in 7.15.0 (and we also fixed our code). Therefore closing the issue :) Thanks @AbhiPrasad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meta: Help Wanted Package: react Issues related to the Sentry React SDK
Projects
None yet
Development

No branches or pull requests

3 participants