nitrojs
Missing beforeResponse hook on some error responses
Environment
[email protected] [email protected]
Reproduction
https://stackblitz.com/edit/unjs-nitro-starter-ppkblahg?file=server%2Froutes%2Fthrow.ts
Describe the bug
Depending on how an error is caused, the beforeResponse hook may not be called.
Here are a few examples from the reproduction
| Error creation | onError | onBeforeResponse | onAfterResponse |
|---|---|---|---|
return new Error() |
yes | yes | no |
throw new Error() |
yes | no | no |
| missing route | yes | no | no |
new Response(null, { status: 503 }) |
no | yes | yes |
I would expect the beforeResponse hook to be called even for missing routes or thrown errors to allow intercepting those responses.
Digging through the code, it looks like the root cause may be that nitro's onError handler actually sends the response https://github.com/nitrojs/nitro/blob/v2/src/runtime/error.ts#L82
This marks the event as handled, so h3 will not call the beforeResponse/afterResponse hooks https://github.com/unjs/h3/blob/v1/src/adapters/node.ts#L73
Maybe a solution could be to also trigger those hooks within the error handler, before and after the send calls.
Additional context
Follow up from https://github.com/nitrojs/nitro/issues/2560
Logs
Thank you so much for spending time to make this info.
In h3 v1 / nitro v2
There is a PR to change the order of calling hooks onBeforeResponse > onError (#793) , which, while it is a completely valid fix, also changes behavior, and at this point of v1 adoption, it is way risky to change it at least without extensive ecosystem tests.
For Nitro v2, (with h3 v1), I like your idea could instead trigger the necessary hook from within the error handler. Since Nitro 2.11 it is possible to stack multiple error handlers for this purpose.
In h3 v2 / nitro v3
onAfterResponse is not supported in h3 v2 since send() does not exist outside of Node.js runtime (and for Node.js runtime, we can use event.node.res.on("..", () => {}) as another workaround)
I think what could be best to investigate and possibly fix any issues before h3 v2 release is that we make sure onBeforeResponse is called all the times (for normal, empty, error or 404 responses).
There is a PR to change the order of calling hooks onBeforeResponse > onError (https://github.com/nitrojs/nitro/pull/793) , which, while it is a completely valid fix, also changes behavior, and at this point of v1 adoption, it is way risky to change it at least without extensive ecosystem tests.
Yeah I saw this PR, and understand that it may be risky at this point. Since this issue a result of the combination of this behavior plus nitro's error handler I thought it made sense to report here first.
For Nitro v2, (with h3 v1), I like your idea could instead trigger the necessary hook from within the error handler. Since Nitro 2.11 it is possible to stack multiple error handlers for this purpose.
Are you open to a PR for this change or do you mean this should be done within applications?
I think what could be best to investigate and possibly fix any issues before h3 v2 release is that we make sure onBeforeResponse is called all the times (for normal, empty, error or 404 responses).
Yeah that makes sense.
Maybe we could also clarify the expected behavior of the error hook? My original assumption was that it was the equivalent to beforeResponse for error responses, but then I looked through the code and saw it was not the case. Perhaps it could be documented that this hook is best for logging errors, but if you need to actually make changes to the response use beforeResponse and check the status. Otherwise you run into issues like https://github.com/nitrojs/nitro/issues/2771
The other question I had is should the error hook also be called for something like new Response(null, { status: 503 })? Or is it just for JS errors, either thrown or returned.