fix(nextjs): Revert #4139 - remove manipulation of res.finished value
#4516
Conversation
size-limit report
|
| if (process.env.NODE_ENV === 'development') { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| '\n[sentry] If Next.js logs a warning "API resolved without sending a response", it\'s a false positive, which we\'re working to rectify.\n', |
There was a problem hiding this comment.
We can’t use the logger here?
Also why are we emitting a new line at the start?
There was a problem hiding this comment.
The logger only shows up if the user has debug: true in their config, whereas the warning our message references shows up regardless.
As for the newline, in my testing the message was just kind of getting lost in all the logs, so I stuck some blank lines around it. Do you think it's an issue?
There was a problem hiding this comment.
I would prefer to remove the new lines to reduce the noise caused by sentry.
That’s primarily my worry here with the console.warn, that it will cause a lot of noise in the development environment (that can’t be turned off). With debug logging, it’s opt in from the user.
There was a problem hiding this comment.
I can take the newlines out, but bottom line, this creates exactly as much noise as the warning we're causing. It's just that with this logging, it's now
[sentry] There's about to be a warning - it's fine.
Warning - things might be broken.
rather than
Warning - things might be broken.
As a user, I'd rather have that be effectively a two-line warning, including reassurance that I don't need to stress, rather than a one-line warning whose origin and importance I have to wonder about (unless I happen to have turned on debugging in Sentry, which the warning itself gives me no hint that I'd need to do).
The least noise, of course, would be if we weren't causing the warning in the first place, and that's what the original PR accomplished. But given that for the moment we're choosing annoying warning over potential breakage, I really do think it's better to at least let people know why it's there and that it's safe to ignore. Don't you?
There was a problem hiding this comment.
Hello @AbhiPrasad @lobsterkatie, how are you? Is the potential fix being tracked in an issue? Combined with next/auth and swr for fetching the user profile it's triggering a massive amount of logs.
Thank you for your hard work 👍
There was a problem hiding this comment.
Hey, @Fedeorlandau thanks for reaching out. Could you please open an issue with your setup details? (SDK version, next version, next config), it'll help a ton for us to debug.
lobsterkatie
deleted the
kmclb-nextjs-remove-hack-marking-res-finished
branch
…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)
In #4139, a change was introduced in order to suppress a false positive warning thrown by nextjs, by setting
res.finishedtotruejust 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 #3852 and #4007), 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.