-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(astro): Automatically add Sentry middleware in Astro integration #9532
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
size-limit report 📦
|
// Users on older versions of astro will need to add the middleware manually. | ||
const supportsAddMiddleware = typeof addMiddleware === 'function'; | ||
|
||
if (supportsAddMiddleware && isSSR && shouldAddMiddleware) { |
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 thinking, will this break if people update the SDK & astro to the latest version and already have the middleware added manually - e.g. will it then add the middleware twice? or do middlewares (or our middleware) handle this scenario?
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.
Good point!
will this break if people update the SDK
Technically, we're still in alpha so I'd be fine with this. Furthermore, middleware wasn't yet documented anywhere except for the readme.
However, for general robustness, we can probably detect and handle double wrapping by writing a flag to the locals
object that's passed into the middleware handlers. I'll check take a look.
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.
if this is not documented yet, IMHO let's just break it! no need to introduce additional complexity then 👍
The bug in Astro was fixed in withastro/astro#9057 (released with version 3.5.2), so we can now review and merge this. Also tested that it works locally in a test app 👍 |
8e3a32c
to
2c08271
Compare
This PR adds automatic registration of our Astro middleware. This is possible since Astro 3.5.2 by adding middleware entry points in the astro integration's setup hook.
Updated Readme
This is backwards compatible with previous Astro versions because we can simply check if the
addMiddleware
function exists and only make use of it if it does.ref #9444