fix(nextjs): Prevent false API resolved without sending a response warning
#4139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report
|
iker-barriocanal
approved these changes
Nov 10, 2021
lobsterkatie
force-pushed
the
kmclb-nextjs-prevent-no-response-warning-in-dev
branch
from
0a70206
to
39c1e7b
Compare
lobsterkatie
deleted the
kmclb-nextjs-prevent-no-response-warning-in-dev
branch
5 tasks
lobsterkatie
added a commit
that referenced
this pull request
Feb 8, 2022
…ue (#4516) In #4139, a change was introduced in order to suppress a false positive warning thrown by nextjs, by setting `res.finished` to `true` just long enough for nextjs to check it and decide no warning was needed. In simple cases, it worked just fine, but in some set-ups the "just long enough for nextjs to check it and calm down" turned out to also be "just long enough for other things to check it and get mad." This backs out that change, as it seems it's doing more harm than good. In order to address the original problem (documented in [1] and [2]), a new warning is added, explaining that the false positive is just that. Not as elegant as suppressing the message altogether, but it should tide us over until such time as we're able to try again with a different approach. Fixes #4151. [1] #3852 [2] #4007
lobsterkatie
added a commit
that referenced
this pull request
Mar 11, 2022
…4706) In the nextjs SDK, when we wrap users' API routes, we also wrap the response's `end` method, in order to keep the lambda running the route handler alive long enough to send events to Sentry. As a consequence, however, Next thinks a response hasn't been sent at all, because `end` hasn't been called within the timeframe that it expects, so it throws a warning in dev. A previous attempt[1] to fix this problem backfired[2], and had to be reverted[3], so as a compromise option for the moment, we log a warning about the Next warning, so at least people know it's nothing to worry about. Some people are finding this behavior spammy[4], though, so this PR adds an env variable check which allows a user to suppress our meta-warning, and also improves the warning itself so that people know the option is there. [1] #4139 [2] #4151 [3] #4516 [4] #3852 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR prevents a false positive warning from nextjs which arises from the interaction of our API route wrapper
withSentry()with nextjs's native API route handling.In dev, nextjs checks that API route handlers return a response to the client before they resolve, and it throws a warning if this hasn't happened. Meanwhile, in
withSentry(), we wrap theres.end()method to ensure that events are flushed before the request/response lifecycle finishes.As a result, there are cases where the handler resolves before the response is finished, while flushing is still in progress. This triggers the warning mentioned above, but it's a false alarm - the response may not have ended yet, but it will end. This suppresses the warning by temporarily marking the response finished, and then restoring it to unfinished before the original
res.end()is called.The fix here is simple, but the async-i-ness of it all leads to some complex interactions in sequencing between the SDK, nextjs, and Node itself. I've tried to lay it out as clearly as I can in comments.
Fixes #4007
Fixes #3852