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

fix(tracing): Don't finish React Router 6 pageload transactions early #6609

Merged
merged 6 commits into from
Jan 4, 2023
Merged

fix(tracing): Don't finish React Router 6 pageload transactions early #6609

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e409e5e
fix(tracing): Don't finish React Router 6 `pageload` transactions early
onurtemizkan Dec 21, 2022
8c7b2e7
Follow react patterns
lforst Dec 22, 2022
9af81a0
fix logic
lforst Dec 22, 2022
7aee49e
clean
lforst Dec 22, 2022
81b57cf
Clean again
lforst Jan 3, 2023
af715ba
Use boolean
lforst Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 36 additions & 39 deletions packages/react/src/reactrouterv6.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -179,28 +170,28 @@ export function withSentryReactRouterV6Routing<P extends Record<string, any>, R
return Routes;
}

let isBaseLocation: boolean = false;
let routes: RouteObject[];
let previousLocation: Location | null = null;
Copy link
Member

Choose a reason for hiding this comment

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

l: As discussed we can change this to a boolean

Copy link
Member Author

Choose a reason for hiding this comment

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

af715ba


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[];
isBaseLocation = true;

updatePageloadTransaction(location, routes);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.children]);
_useEffect(
() => {
const routes = _createRoutesFromChildren(props.children) as RouteObject[];

_useEffect(() => {
handleNavigation(location, routes, navigationType, isBaseLocation);
}, [props.children, location, navigationType, isBaseLocation]);
if (previousLocation === null) {
updatePageloadTransaction(location, routes);
} else {
handleNavigation(location, routes, navigationType);
}

isBaseLocation = false;
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
Expand All @@ -224,28 +215,34 @@ export function wrapUseRoutes(origUseRoutes: UseRoutes): UseRoutes {
return origUseRoutes;
}

let isBaseLocation: boolean = false;
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);

const locationArgObject = typeof location === 'string' ? { pathname: location } : location;
const locationObject = (locationArgObject as Location) || _useLocation();
const location = _useLocation();
const navigationType = _useNavigationType();

_useEffect(() => {
isBaseLocation = true;

updatePageloadTransaction(locationObject, routes);
}, [props]);
// 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(() => {
handleNavigation(locationObject, routes, navigationType, isBaseLocation);
}, [props, locationObject, navigationType, isBaseLocation]);
const normalizedLocation =
typeof stableLocationParam === 'string' ? { pathname: stableLocationParam } : stableLocationParam;

if (previousLocation === null) {
updatePageloadTransaction(normalizedLocation, routes);
} else {
handleNavigation(normalizedLocation, routes, navigationType);
}

isBaseLocation = false;
previousLocation = normalizedLocation;
}, [navigationType, stableLocationParam]);

return Routes;
};
Expand Down Expand Up @@ -275,7 +272,7 @@ export function wrapCreateBrowserRouter(createRouterFunction: CreateRouterFuncti
(state.historyAction === 'PUSH' || state.historyAction === 'POP') &&
activeTransaction
) {
handleNavigation(location, routes, state.historyAction, false);
handleNavigation(location, routes, state.historyAction);
}
});

Expand Down