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

experimental: error callback for middleware #1280

Merged
merged 6 commits into from
Mar 7, 2025
Merged

experimental: error callback for middleware #1280

merged 6 commits into from
Mar 7, 2025

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Mar 2, 2025

(I'm still not sure how onError works. Isn't it documented?)

Copy link

vercel bot commented Mar 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Mar 6, 2025 11:52pm

Copy link

codesandbox-ci bot commented Mar 2, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@rmarscher
Copy link
Collaborator

There is documentation for onError in the parameters section of renderToReadableStream

onError: A callback that fires whenever there is a server error, whether recoverable or not. By default, this only calls console.error. If you override it to log crash reports, make sure that you still call console.error. You can also use it to adjust the status code before the shell is emitted.

We should add console.error(err); to onError. I see that is missing from Waku's implementation.

Each of these usage sections discuss how onError can be used:

Logging crashes on the server is the primary use case that I have for onError. I have server-side console.error hooked up to my error tracking - so really I won't have to add anything else if we add that. But... for people that want to do their own instrumentation and maybe ignore certain errors, it would be really great to register a callback to do that.

In those React usage docs, the methods for setting status code or making the onError error accessible to the request handler are interesting... but as soon as there is a suspense boundary, they won't work as expected because the onError will happen after the response starts streaming.

If I want to add error reporting for everything collected by ctx.unstable_errs - how can I do that? Create Waku middleware and pipe the ctx.body through my own transform that reports new ctx.unstable_errs as they become available and then one final check when the stream is complete?

@wesbos wesbos mentioned this pull request Mar 3, 2025
@rmarscher
Copy link
Collaborator

Isn't it documented?

There is documentation for onError in the parameters section of renderToReadableStream

Sorry... I realize I was confusing react-dom/server renderToReadableStream with react-server-dom-webpack renderToReadableStream. I wonder if they are intended to work in a similar way?

@dai-shi
Copy link
Member Author

dai-shi commented Mar 4, 2025

Yeah, I've read the react-dom docs once before.

We should add console.error(err); to onError. I see that is missing from Waku's implementation.

Nice catch.

If I want to add error reporting for everything collected by ctx.unstable_errs - how can I do that? Create Waku middleware and pipe the ctx.body through my own transform that reports new ctx.unstable_errs as they become available and then one final check when the stream is complete?

Yes, my expectation is write your own middleware. I haven't tried anything and not sure if it works as you expect. Feedback is welcome.

@dai-shi dai-shi marked this pull request as ready for review March 4, 2025 02:49
@dai-shi dai-shi changed the title experimental: errors in ctx experimental: error callback for middleware Mar 5, 2025
Copy link
Collaborator

@rmarscher rmarscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think it is ready to merge unless you aren't sure that you like how the callbacks are being passed through.

@dai-shi
Copy link
Member Author

dai-shi commented Mar 6, 2025

Thanks for the review. I'm not 100% confident, but as it's marked as unstable_, I'm fine to merge.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@dai-shi dai-shi merged commit a1185ac into main Mar 7, 2025
26 checks passed
@dai-shi dai-shi deleted the fix/error-in-ctx branch March 7, 2025 02:40
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.

None yet

2 participants