Skip to content
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 error reporting #114

Closed

Conversation

StNekroman
Copy link

@StNekroman StNekroman commented Nov 14, 2024

Problem:

If exception raises on Nest side - server doesn't flush any response to client at all.
Network request in browser just spins, because Next code deadlock on this.ready inside NextCustomServer impl.
Here was issue: vercel/next.js#54977
One highlight from that issue: we cannot use "renderError" method anyway - because somebody (who has permissions to close tickets) says that it's "not public", while typescript typings says that it's public.

Anyway, when you set: passthrough404: true 404 error will be rendered by Next - everything fine.
But any other error will get stuck. For example NPE --> unflushed request.

If you set passthrough404: false - then just all errors stuck on server.

So idea of fix is simple: pass error handling to Nest itself, let if deal with that.
Because before this fix, in RenderModule there was this.service.setErrorRenderer(next.renderError.bind(next));
So effectively you was passing error to Next, as final fallback.

After this fix, I just call default exception handler's impl - BaseExceptionFilter.
Ideally I would make RenderFilter extends BaseExceptionFilter and just do super.catch(...) inside.
But that would be bigger refactoring and there is already very helpful renderService.getErrorRenderer for some reason.

@StNekroman
Copy link
Author

@kyle-mccarthy take a look on this, please.
Additionally I want say that passthrough404 is hack.
What if I want different routing (of error handling) depending on path?
For example - if /api/** - then use Nest rendering (just plain http error code + error payload)
if not /api/** - then pass to Next.
With current approach I cannot do that.
So I suggest to replace passthrough404: boolean with passthroughErrors: () => boolean
Bump version to 11.0.0 and declare in doc migrate stop passthrough404: true ==> passthrough404: () => true

@StNekroman
Copy link
Author

Ah, nevermind

@StNekroman StNekroman closed this Nov 19, 2024
@StNekroman StNekroman deleted the fix-error-reporting branch November 19, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant