-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(v8/react): Add support for React Router sub-routes from handle
(#17277)
#17433
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
base: v8
Are you sure you want to change the base?
Conversation
function processResolvedRoutes(resolvedRoutes: RouteObject[], parentRoute?: RouteObject): void { | ||
resolvedRoutes.forEach(child => { | ||
allRoutes.add(child); | ||
// Only check for async handlers if the feature is enabled | ||
if (_enableAsyncRouteHandlers) { | ||
checkRouteForAsyncHandler(child); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The processResolvedRoutes
function lacks cycle detection, which can cause infinite recursion and a stack overflow if a route handler returns a circular reference.
-
Description: The
processResolvedRoutes
function iterates through resolved routes to check for async handlers. However, it lacks a check to see if a route has already been processed. If an async route handler's result contains a circular reference (e.g., a route that is an ancestor in the routing tree), the function will enter an infinite recursion loop (processResolvedRoutes
->checkRouteForAsyncHandler
->handleAsyncHandlerResult
->processResolvedRoutes
). This leads to a stack overflow, crashing the application. -
Suggested fix: Add a guard within the
forEach
loop inprocessResolvedRoutes
to check if a route has already been processed before callingcheckRouteForAsyncHandler
. This can be done by checkingif (!allRoutes.has(child))
, mirroring the protective logic found ingetChildRoutesRecursively
.
severity: 0.85, confidence: 0.9
Did we get this right? 👍 / 👎 to inform future reviews.
for (const key of Object.keys(route.handle)) { | ||
const maybeFn = route.handle[key]; | ||
if (typeof maybeFn === 'function' && !(maybeFn as { __sentry_proxied__?: boolean }).__sentry_proxied__) { | ||
route.handle[key] = createAsyncHandlerProxy(maybeFn, route, key); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The checkRouteForAsyncHandler
function proxies all functions in a route's handle
, not just async loaders, causing significant performance overhead due to a logical flaw.
-
Description: The
checkRouteForAsyncHandler
function indiscriminately proxies every function found within a route'shandle
object. Thehandle
object is often used for various purposes, including synchronous utility functions or metadata generators, not just the intended async route loaders. Every call to any of these proxied functions incurs unnecessary overhead from the proxy'sapply
trap and the subsequent call tohandleAsyncHandlerResult
. Because this logic is applied recursively to the entire route tree, it can cause significant performance degradation in applications with complex routing. -
Suggested fix: Refine the logic in
checkRouteForAsyncHandler
to only proxy functions that are intended to be async route handlers. This could be achieved by checking for a specific naming convention for the handler key or introducing a mechanism to opt-in specific functions for proxying, rather than proxying all functions by default.
severity: 0.7, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
Backports #17277 to v8