-
-
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
Conversation
72df08c
to
5e48038
Compare
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.
Just a few questions -- nice work!
@@ -245,7 +245,7 @@ test.describe('nested SSR routes (client, server, server request)', () => { | |||
|
|||
// Server HTTP request transaction - should be parametrized (todo: currently not parametrized) | |||
expect(serverHTTPServerRequestTxn).toMatchObject({ | |||
transaction: 'GET /api/user/myUsername123.json', // todo: should be parametrized to 'GET /api/user/[userId].json' | |||
transaction: 'GET /api/user/[userId].json', |
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
and user/someID
can be differentiated easily by matching the routePattern
. routePattern
will be user/settings
or user/[userid]
(all lowercased).
const interpolatedRoute = | ||
foundRoute?.patternCaseSensitive || interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params); |
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.
also just so that I understand correctly: The ctx.routePattern
alone is case-insensitive and we use the build time-injected information so that we get the case-sensitive one?
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.
yes
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.
Thanks for clarifying!
(I'll leave reviewing the cursor stuff up to you. Pretty sure some of the findings don't apply).
Addressing cursor comments:
That's no problem during build-time as the hook is only called once. During runtime I'll add a variable to make sure it only runs once.
That's why I am appending
No because the
No, only Astro version lower than 5 |
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.
Bug: Sentry Route Info Repeatedly Appended
The astro:routes:resolved
hook unconditionally appends globalThis["__sentryRouteInfo"]
to the user's server init file. The didSaveRouteData
flag only prevents multiple writes within a single build process, causing route information to be repeatedly appended across multiple builds. This leads to file bloat, duplicate assignments, and potential JavaScript syntax errors or unexpected runtime behavior.
packages/astro/src/integration/index.ts#L174-L203
sentry-javascript/packages/astro/src/integration/index.ts
Lines 174 to 203 in b4ec0f7
// @ts-expect-error - This hook is available in Astro 5+ | |
'astro:routes:resolved': ({ routes }: { routes: IntegrationResolvedRoute[] }) => { | |
if (!sentryServerInitPath || didSaveRouteData) { | |
return; | |
} | |
try { | |
const serverInitContent = readFileSync(sentryServerInitPath, 'utf8'); | |
const updatedServerInitContent = `${serverInitContent}\nglobalThis["__sentryRouteInfo"] = ${JSON.stringify( | |
routes.map(route => { | |
return { | |
...route, | |
patternCaseSensitive: joinRouteSegments(route.segments), // Store parametrized routes with correct casing on `globalThis` to be able to use them on the server during runtime | |
patternRegex: route.patternRegex.source, // using `source` to be able to serialize the regex | |
}; | |
}), | |
null, | |
2, | |
)};`; | |
writeFileSync(sentryServerInitPath, updatedServerInitContent, 'utf8'); | |
didSaveRouteData = true; // Prevents writing the file multiple times during runtime | |
debug.log('Successfully added route pattern information to Sentry config file:', sentryServerInitPath); | |
} catch (error) { | |
debug.warn(`Failed to write to Sentry config file at ${sentryServerInitPath}:`, error); | |
} | |
}, |
Was this report helpful? Give feedback by reacting with 👍 or 👎
) Route Parametrization for Server Requests in Astro 5. The route information is gathered during build-time. During runtime, the route information is matched to use the parametrized route information during runtime Part of #16686
Route Parametrization for Server Requests in Astro 5.
The route information is gathered during build-time. During runtime, the route information is matched to use the parametrized route information during runtime
Part of #16686