-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(astro): Implement Request Route Parametrization for Astro 5 #17105
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
Changes from 2 commits
5e48038
abf9b5f
3a36e1a
78dff8a
94e18a7
b4ec0f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import { | |
withIsolationScope, | ||
} from '@sentry/node'; | ||
import type { APIContext, MiddlewareResponseHandler } from 'astro'; | ||
import type { ResolvedRouteWithCasedPattern } from '../integration/types'; | ||
|
||
type MiddlewareOptions = { | ||
/** | ||
|
@@ -95,6 +96,9 @@ async function instrumentRequest( | |
addNonEnumerableProperty(locals, '__sentry_wrapped__', true); | ||
} | ||
|
||
const storedBuildTimeRoutes = (globalThis as unknown as { __sentryRouteInfo?: ResolvedRouteWithCasedPattern[] }) | ||
?.__sentryRouteInfo; | ||
|
||
const isDynamicPageRequest = checkIsDynamicPageRequest(ctx); | ||
|
||
const request = ctx.request; | ||
|
@@ -128,7 +132,13 @@ async function instrumentRequest( | |
} | ||
|
||
try { | ||
const interpolatedRoute = interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params); | ||
const contextWithRoutePattern = ctx as Parameters<MiddlewareResponseHandler>[0] & { routePattern?: string }; | ||
const rawRoutePattern = contextWithRoutePattern.routePattern; | ||
Lms24 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const foundRoute = storedBuildTimeRoutes?.find(route => route.pattern === rawRoutePattern); | ||
|
||
const interpolatedRoute = | ||
foundRoute?.patternCaseSensitive || interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also just so that I understand correctly: The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
const source = interpolatedRoute ? 'route' : 'url'; | ||
// storing res in a variable instead of directly returning is necessary to | ||
// invoke the catch block if next() throws | ||
|
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.
l: Is there a test that ensures that a known route like
GET / api/user/settings.json
? Just wanna make sure we can disambiguate these routes :)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.
not yet, but I was thinking about that!
In this case, it would work as
user/settings
anduser/someID
can be differentiated easily by matching theroutePattern
.routePattern
will beuser/settings
oruser/[userid]
(all lowercased).