-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Let flush
finish in API routes
#3811
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
fix(nextjs): Let flush
finish in API routes
#3811
Conversation
size-limit report
|
I went through the broken integration tests. They are all failing because of |
Thanks!
Yup, I know that's the underlying problem. But even once I fixed the tests, one is still failing, for mysterious reasons. ¯\(ツ)/¯ I'm sure I'll figure it out. That and the linting stuff I just ran out of time to fix before I turned into a pumpkin last night. |
db7b735
to
d1f1824
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.
Had a partial look because @AbhiPrasad asked :)
I'd beg to avoid adding more API surface to general packages.
I understand scope bleed, but that's a much larger topic, I wouldn't try to mix that and the flush
fix in one PR.
Recording the results of an IRL convo with @rhcarvalho, @AbhiPrasad, and @kamilogorek. The scope issue is sufficiently complicated (and wide-reaching) that it will be pushed off to another PR. Here, we'll dial it back to just keeping track of the open transaction. |
4657ca7
to
8edb6f6
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.
Nice!
8edb6f6
to
a4763f5
Compare
As of getsentry/sentry-javascript#3811, the SDK should now work in API routes.
I don't quite understand this - Vercel/Next.js supports return promises. Because of this monkey patching I'm getting: |
As discussed in more detail in the penultimate paragraph of this PR, when deployed to vercel, API route lambdas have been shutting down too soon for sentry events to get reliably sent to our servers (in spite of the use of
flush
). This fixes that by wrapping the response's.end()
method (which is what triggers the lambda shutdown) such that it waits for flush to finish before emitting itsfinished
signal.Logs from API routes now consistently look like this:
Fixes #3806
Fixes #3643
Fixes #3691
Fixes #3748
Fixes #3636