[pull] canary from vercel:canary #929
Merged
Conversation
This file contains hidden or 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
…77746) This PR fixes some bugs in our handling of `serverActions.bodySizeLimit`. There's a couple fixes here, which can be viewed commit by commit. ### 1. Uncaught exception error when exceeding the size limit We were using `body.pipe(busboy)`, which does not forward errors from the source to the sink, so the busboy stream would just hang, and we'd log an error, but never produce a response. It seems like node is (usually) smart enough to abort the request when this happens (because it was triggering an uncaught exception), but we should be returning a proper response instead. This is fixed by using `require('node:stream').pipeline(body, busboy)`, which propagates errors correctly. ### 2. Apply size limit to non-multipart fetch actions We have a separate codepath for non-multipart fetch actions. This can happen if the action arguments are simple enough that react doesn't need to use multiple rows to serialize them -- generally, this means that they're JSON-esque without any complex types like Promises, Maps, or Sets. This codepath was consuming the original request body, not the one piped through the size limit transform. So we'd print the error (because the transform is subscribed to the body), but still execute the action. ### 3. Tests The likely reason that these bugs slipped through is that the tests were only checking if an error was printed to the console, and not checking if the server actually responded with an error. To prevent this, I've added some assertions on the responses' status codes + checking whether an error boundary was triggered. I've also tweaked the tests so that the parts that submit actions can run in deploy mode. The request interception code proved subtle/complicated enough that i decided to pull it out into a separate helper. We have a whole bunch of tests that intercept requests/responses using various ad-hoc tangles of `page/browser.on('request', ...)` which i'd like to migrate to this helper, because that'd make them easier to understand, but I'll leave that for a follow up when there's time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.1)
Can you help keep this open source service alive? 💖 Please sponsor : )