From e409e5ecd580f73ea02e06dff252ac49a6389f4e Mon Sep 17 00:00:00 2001
From: Onur Temizkan <onur@narval.co.uk>
Date: Wed, 21 Dec 2022 13:54:40 +0000
Subject: [PATCH 1/6] fix(tracing): Don't finish React Router 6 `pageload`
transactions early
---
packages/react/src/reactrouterv6.tsx | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx
index 4635296a6bb8..2c1ddef69478 100644
--- a/packages/react/src/reactrouterv6.tsx
+++ b/packages/react/src/reactrouterv6.tsx
@@ -131,17 +131,8 @@ function handleNavigation(
location: Location,
routes: RouteObject[],
navigationType: Action,
- isBaseLocation: boolean,
matches?: AgnosticDataRouteMatch,
): void {
- if (isBaseLocation) {
- if (activeTransaction) {
- activeTransaction.finish();
- }
-
- return;
- }
-
const branches = Array.isArray(matches) ? matches : _matchRoutes(routes, location);
if (_startTransactionOnLocationChange && (navigationType === 'PUSH' || navigationType === 'POP') && branches) {
@@ -179,12 +170,12 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
return Routes;
}
- let isBaseLocation: boolean = false;
let routes: RouteObject[];
const SentryRoutes: React.FC<P> = (props: P) => {
const location = _useLocation();
const navigationType = _useNavigationType();
+ let isBaseLocation: boolean = false;
_useEffect(() => {
// Performance concern:
@@ -197,7 +188,9 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
}, [props.children]);
_useEffect(() => {
- handleNavigation(location, routes, navigationType, isBaseLocation);
+ if (!isBaseLocation) {
+ handleNavigation(location, routes, navigationType);
+ }
}, [props.children, location, navigationType, isBaseLocation]);
isBaseLocation = false;
@@ -224,8 +217,6 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
return origUseRoutes;
}
- let isBaseLocation: boolean = false;
-
// eslint-disable-next-line react/display-name
return (routes: RouteObject[], location?: Partial<Location> | string): React.ReactElement | null => {
const SentryRoutes: React.FC<unknown> = (props: unknown) => {
@@ -234,6 +225,7 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
const locationArgObject = typeof location === 'string' ? { pathname: location } : location;
const locationObject = (locationArgObject as Location) || _useLocation();
const navigationType = _useNavigationType();
+ let isBaseLocation: boolean = false;
_useEffect(() => {
isBaseLocation = true;
@@ -242,7 +234,9 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
}, [props]);
_useEffect(() => {
- handleNavigation(locationObject, routes, navigationType, isBaseLocation);
+ if (!isBaseLocation) {
+ handleNavigation(locationObject, routes, navigationType);
+ }
}, [props, locationObject, navigationType, isBaseLocation]);
isBaseLocation = false;
@@ -275,7 +269,7 @@ export function wrapCreateBrowserRouter(createRouterFunction: CreateRouterFuncti
(state.historyAction === 'PUSH' || state.historyAction === 'POP') &&
activeTransaction
) {
- handleNavigation(location, routes, state.historyAction, false);
+ handleNavigation(location, routes, state.historyAction);
}
});
From 8c7b2e75fd3e4b2bdcc0e6ff62b78dee3566d2d6 Mon Sep 17 00:00:00 2001
From: Luca Forstner <luca.forstner@sentry.io>
Date: Thu, 22 Dec 2022 14:19:56 +0000
Subject: [PATCH 2/6] Follow react patterns
---
packages/react/src/reactrouterv6.tsx | 47 ++++++++++++++++++----------
1 file changed, 30 insertions(+), 17 deletions(-)
diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx
index 2c1ddef69478..8bd573a9bdb9 100644
--- a/packages/react/src/reactrouterv6.tsx
+++ b/packages/react/src/reactrouterv6.tsx
@@ -171,29 +171,35 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
}
let routes: RouteObject[];
+ let isFirstPageloadUpdateUseEffectCall = true;
+ let isFirstNavigationUseEffectCall = true;
const SentryRoutes: React.FC<P> = (props: P) => {
const location = _useLocation();
const navigationType = _useNavigationType();
- let isBaseLocation: boolean = false;
_useEffect(() => {
// Performance concern:
// This is repeated when <Routes /> is rendered.
routes = _createRoutesFromChildren(props.children) as RouteObject[];
- isBaseLocation = true;
-
- updatePageloadTransaction(location, routes);
- // eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.children]);
_useEffect(() => {
- if (!isBaseLocation) {
+ if (isFirstNavigationUseEffectCall) {
+ isFirstNavigationUseEffectCall = false;
+ } else {
handleNavigation(location, routes, navigationType);
}
- }, [props.children, location, navigationType, isBaseLocation]);
+ }, [location, navigationType]);
- isBaseLocation = false;
+ _useEffect(() => {
+ if (isFirstPageloadUpdateUseEffectCall) {
+ routes = _createRoutesFromChildren(props.children) as RouteObject[];
+ updatePageloadTransaction(location, routes);
+ } else {
+ isFirstPageloadUpdateUseEffectCall = false;
+ }
+ }, [props.children, location]);
// @ts-ignore Setting more specific React Component typing for `R` generic above
// will break advanced type inference done by react router params
@@ -217,29 +223,36 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
return origUseRoutes;
}
+ let isFirstPageloadUpdateUseEffectCall = true;
+ let isFirstNavigationUseEffectCall = true;
+
// eslint-disable-next-line react/display-name
return (routes: RouteObject[], location?: Partial<Location> | string): React.ReactElement | null => {
const SentryRoutes: React.FC<unknown> = (props: unknown) => {
const Routes = origUseRoutes(routes, location);
+ // the case where location is a string might be killing performance because we're always creating a new
+ // object(with different identity) that will trigger useEffects below to run again
const locationArgObject = typeof location === 'string' ? { pathname: location } : location;
+
const locationObject = (locationArgObject as Location) || _useLocation();
const navigationType = _useNavigationType();
- let isBaseLocation: boolean = false;
_useEffect(() => {
- isBaseLocation = true;
-
- updatePageloadTransaction(locationObject, routes);
- }, [props]);
+ if (isFirstPageloadUpdateUseEffectCall) {
+ updatePageloadTransaction(locationObject, routes);
+ } else {
+ isFirstPageloadUpdateUseEffectCall = false;
+ }
+ }, [locationObject, routes]);
_useEffect(() => {
- if (!isBaseLocation) {
+ if (isFirstNavigationUseEffectCall) {
handleNavigation(locationObject, routes, navigationType);
+ } else {
+ isFirstNavigationUseEffectCall = false;
}
- }, [props, locationObject, navigationType, isBaseLocation]);
-
- isBaseLocation = false;
+ }, [locationObject, routes, navigationType]);
return Routes;
};
From 9af81a018f0a325625c7a1149395e135acc5f244 Mon Sep 17 00:00:00 2001
From: Luca Forstner <luca.forstner@sentry.io>
Date: Thu, 22 Dec 2022 14:22:28 +0000
Subject: [PATCH 3/6] fix logic
---
packages/react/src/reactrouterv6.tsx | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx
index 8bd573a9bdb9..f833888625bb 100644
--- a/packages/react/src/reactrouterv6.tsx
+++ b/packages/react/src/reactrouterv6.tsx
@@ -194,10 +194,9 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
_useEffect(() => {
if (isFirstPageloadUpdateUseEffectCall) {
+ isFirstPageloadUpdateUseEffectCall = false;
routes = _createRoutesFromChildren(props.children) as RouteObject[];
updatePageloadTransaction(location, routes);
- } else {
- isFirstPageloadUpdateUseEffectCall = false;
}
}, [props.children, location]);
@@ -240,17 +239,16 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
_useEffect(() => {
if (isFirstPageloadUpdateUseEffectCall) {
- updatePageloadTransaction(locationObject, routes);
- } else {
isFirstPageloadUpdateUseEffectCall = false;
+ updatePageloadTransaction(locationObject, routes);
}
}, [locationObject, routes]);
_useEffect(() => {
if (isFirstNavigationUseEffectCall) {
- handleNavigation(locationObject, routes, navigationType);
- } else {
isFirstNavigationUseEffectCall = false;
+ } else {
+ handleNavigation(locationObject, routes, navigationType);
}
}, [locationObject, routes, navigationType]);
From 7aee49e7faccb238339bdc8a687761418cca55cf Mon Sep 17 00:00:00 2001
From: Luca Forstner <luca.forstner@sentry.io>
Date: Thu, 22 Dec 2022 14:22:39 +0000
Subject: [PATCH 4/6] clean
---
packages/react/src/reactrouterv6.tsx | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx
index f833888625bb..cb5298cf7a92 100644
--- a/packages/react/src/reactrouterv6.tsx
+++ b/packages/react/src/reactrouterv6.tsx
@@ -184,14 +184,6 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
routes = _createRoutesFromChildren(props.children) as RouteObject[];
}, [props.children]);
- _useEffect(() => {
- if (isFirstNavigationUseEffectCall) {
- isFirstNavigationUseEffectCall = false;
- } else {
- handleNavigation(location, routes, navigationType);
- }
- }, [location, navigationType]);
-
_useEffect(() => {
if (isFirstPageloadUpdateUseEffectCall) {
isFirstPageloadUpdateUseEffectCall = false;
@@ -200,6 +192,14 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
}
}, [props.children, location]);
+ _useEffect(() => {
+ if (isFirstNavigationUseEffectCall) {
+ isFirstNavigationUseEffectCall = false;
+ } else {
+ handleNavigation(location, routes, navigationType);
+ }
+ }, [location, navigationType]);
+
// @ts-ignore Setting more specific React Component typing for `R` generic above
// will break advanced type inference done by react router params
return <Routes {...props} />;
From 81b57cf9ee7318a2886488a9f8cdbac242337ed8 Mon Sep 17 00:00:00 2001
From: Luca Forstner <luca.forstner@sentry.io>
Date: Tue, 3 Jan 2023 14:39:36 +0000
Subject: [PATCH 5/6] Clean again
---
packages/react/src/reactrouterv6.tsx | 78 +++++++++++++---------------
1 file changed, 35 insertions(+), 43 deletions(-)
diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx
index cb5298cf7a92..cd0825a34a84 100644
--- a/packages/react/src/reactrouterv6.tsx
+++ b/packages/react/src/reactrouterv6.tsx
@@ -170,35 +170,28 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
return Routes;
}
- let routes: RouteObject[];
- let isFirstPageloadUpdateUseEffectCall = true;
- let isFirstNavigationUseEffectCall = true;
+ let previousLocation: Location | null = null;
const SentryRoutes: React.FC<P> = (props: P) => {
const location = _useLocation();
const navigationType = _useNavigationType();
- _useEffect(() => {
- // Performance concern:
- // This is repeated when <Routes /> is rendered.
- routes = _createRoutesFromChildren(props.children) as RouteObject[];
- }, [props.children]);
-
- _useEffect(() => {
- if (isFirstPageloadUpdateUseEffectCall) {
- isFirstPageloadUpdateUseEffectCall = false;
- routes = _createRoutesFromChildren(props.children) as RouteObject[];
- updatePageloadTransaction(location, routes);
- }
- }, [props.children, location]);
+ _useEffect(
+ () => {
+ const routes = _createRoutesFromChildren(props.children) as RouteObject[];
- _useEffect(() => {
- if (isFirstNavigationUseEffectCall) {
- isFirstNavigationUseEffectCall = false;
- } else {
- handleNavigation(location, routes, navigationType);
- }
- }, [location, navigationType]);
+ if (previousLocation === null) {
+ updatePageloadTransaction(location, routes);
+ } else {
+ handleNavigation(location, routes, navigationType);
+ }
+
+ previousLocation = location;
+ },
+ // `props.children` is purpusely not included in the dependency array, because we do not want to re-run this effect
+ // when the children change. We only want to start transactions when the location or navigation type change.
+ [location, navigationType],
+ );
// @ts-ignore Setting more specific React Component typing for `R` generic above
// will break advanced type inference done by react router params
@@ -222,35 +215,34 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
return origUseRoutes;
}
- let isFirstPageloadUpdateUseEffectCall = true;
- let isFirstNavigationUseEffectCall = true;
+ let previousLocation: Location | null = null;
// eslint-disable-next-line react/display-name
- return (routes: RouteObject[], location?: Partial<Location> | string): React.ReactElement | null => {
- const SentryRoutes: React.FC<unknown> = (props: unknown) => {
- const Routes = origUseRoutes(routes, location);
+ return (routes: RouteObject[], locationArg?: Partial<Location> | string): React.ReactElement | null => {
+ const SentryRoutes: React.FC<unknown> = () => {
+ const Routes = origUseRoutes(routes, locationArg);
- // the case where location is a string might be killing performance because we're always creating a new
- // object(with different identity) that will trigger useEffects below to run again
- const locationArgObject = typeof location === 'string' ? { pathname: location } : location;
-
- const locationObject = (locationArgObject as Location) || _useLocation();
+ const location = _useLocation();
const navigationType = _useNavigationType();
- _useEffect(() => {
- if (isFirstPageloadUpdateUseEffectCall) {
- isFirstPageloadUpdateUseEffectCall = false;
- updatePageloadTransaction(locationObject, routes);
- }
- }, [locationObject, routes]);
+ // A value with stable identity to either pick `locationArg` if available or `location` if not
+ const stableLocationParam =
+ typeof locationArg === 'string' || locationArg?.pathname !== undefined
+ ? (locationArg as { pathname: string })
+ : location;
_useEffect(() => {
- if (isFirstNavigationUseEffectCall) {
- isFirstNavigationUseEffectCall = false;
+ const normalizedLocation =
+ typeof stableLocationParam === 'string' ? { pathname: stableLocationParam } : stableLocationParam;
+
+ if (previousLocation === null) {
+ updatePageloadTransaction(normalizedLocation, routes);
} else {
- handleNavigation(locationObject, routes, navigationType);
+ handleNavigation(normalizedLocation, routes, navigationType);
}
- }, [locationObject, routes, navigationType]);
+
+ previousLocation = normalizedLocation;
+ }, [navigationType, stableLocationParam]);
return Routes;
};
From af715ba0f8b1fada65af3961a56e4216cb0e1e42 Mon Sep 17 00:00:00 2001
From: Luca Forstner <luca.forstner@sentry.io>
Date: Tue, 3 Jan 2023 16:49:16 +0000
Subject: [PATCH 6/6] Use boolean
---
packages/react/src/reactrouterv6.tsx | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx
index cd0825a34a84..47db243cd8ab 100644
--- a/packages/react/src/reactrouterv6.tsx
+++ b/packages/react/src/reactrouterv6.tsx
@@ -170,7 +170,7 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
return Routes;
}
- let previousLocation: Location | null = null;
+ let isMountRenderPass: boolean = true;
const SentryRoutes: React.FC<P> = (props: P) => {
const location = _useLocation();
@@ -180,13 +180,12 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
() => {
const routes = _createRoutesFromChildren(props.children) as RouteObject[];
- if (previousLocation === null) {
+ if (isMountRenderPass) {
updatePageloadTransaction(location, routes);
+ isMountRenderPass = false;
} else {
handleNavigation(location, routes, navigationType);
}
-
- previousLocation = location;
},
// `props.children` is purpusely not included in the dependency array, because we do not want to re-run this effect
// when the children change. We only want to start transactions when the location or navigation type change.
@@ -215,7 +214,7 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
return origUseRoutes;
}
- let previousLocation: Location | null = null;
+ let isMountRenderPass: boolean = true;
// eslint-disable-next-line react/display-name
return (routes: RouteObject[], locationArg?: Partial<Location> | string): React.ReactElement | null => {
@@ -235,13 +234,12 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
const normalizedLocation =
typeof stableLocationParam === 'string' ? { pathname: stableLocationParam } : stableLocationParam;
- if (previousLocation === null) {
+ if (isMountRenderPass) {
updatePageloadTransaction(normalizedLocation, routes);
+ isMountRenderPass = false;
} else {
handleNavigation(normalizedLocation, routes, navigationType);
}
-
- previousLocation = normalizedLocation;
}, [navigationType, stableLocationParam]);
return Routes;