feat: support experimental.serverActions.bodySizeLimit from next.config#338
feat: support experimental.serverActions.bodySizeLimit from next.config#338youanden wants to merge 1 commit intocloudflare:mainfrom
Conversation
|
/bigbonk review |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good feature addition — the 1MB hardcoded limit was a real pain point for file uploads. The overall architecture is correct: config parsing → resolved config → plumbed through generateRscEntry → embedded in generated code. The production server and Cloudflare worker entry inherit the limit from the built RSC entry, so they're covered automatically.
Tests pass and cover the main cases well. A few issues to address below, ranging from a behavioral mismatch with Next.js to minor suggestions.
| if (value === undefined || value === null) return 1 * 1024 * 1024; | ||
| if (typeof value === "number") return value; | ||
| if (typeof value !== "string") return 1 * 1024 * 1024; | ||
| const match = value.trim().match(/^(\d+(?:\.\d+)?)\s*(b|kb|mb|gb)$/i); |
There was a problem hiding this comment.
Behavioral mismatch with Next.js: The bytes library that Next.js uses (via require('next/dist/compiled/bytes')) accepts formats that this regex rejects:
- Bare numbers as strings —
"1048576"(no unit) is treated as bytes bybytes.parse(), but this regex requires a unit suffix and would fall back to 1MB. - Space between number and unit —
"10 mb"works inbytes.parse()and\s*in the regex handles this, so that's fine. - Terabytes and petabytes — Next.js's
FileSizeSuffixtype includestb,pb(and their uppercase variants). Thebyteslibrary also supports these. This implementation only handlesb|kb|mb|gb.
The bare-number-as-string case is the most likely to bite users in practice. Consider handling it:
| const match = value.trim().match(/^(\d+(?:\.\d+)?)\s*(b|kb|mb|gb)$/i); | |
| const match = value.trim().match(/^(\d+(?:\.\d+)?)\s*(b|kb|mb|gb|tb|pb)?$/i); | |
| if (!match) return 1 * 1024 * 1024; | |
| const num = parseFloat(match[1]); | |
| const unit = (match[2] || 'b').toLowerCase(); |
This makes the unit optional (defaulting to bytes) and adds tb/pb support. You'd need to add case "tb": return Math.floor(num * 1024 * 1024 * 1024 * 1024); and case "pb" to the switch as well.
| * Accepts Next.js-style strings like "1mb", "500kb", "10mb". | ||
| * Returns the default 1MB if the value is not provided or invalid. | ||
| */ | ||
| export function parseBodySizeLimit(value: string | number | undefined | null): number { |
There was a problem hiding this comment.
Minor: Next.js validates that the parsed value is >= 1 and throws an error for negative numbers or zero (see packages/next/src/server/config.ts). This implementation silently accepts 0, negative numbers, and "0mb" as valid. Consider adding validation:
if (value <= 0) return 1 * 1024 * 1024;or even throwing, to match Next.js behavior:
if (value === null || isNaN(value) || value < 1) {
throw new Error(
'Server Actions Size Limit must be a valid number or filesize format larger than 1MB: https://nextjs.org/docs/app/api-reference/next-config-js/serverActions#bodysizelimit'
);
}Not a blocker, but something to consider for parity.
| export function parseBodySizeLimit(value: string | number | undefined | null): number { | ||
| if (value === undefined || value === null) return 1 * 1024 * 1024; | ||
| if (typeof value === "number") return value; | ||
| if (typeof value !== "string") return 1 * 1024 * 1024; |
There was a problem hiding this comment.
Nit: This branch is unreachable given the TypeScript signature string | number | undefined | null. At runtime with untyped JS it could theoretically be hit, but given that the caller casts to string | number | undefined, this is dead code. Not worth blocking on, just noting it.
| it("returns default 1MB for invalid strings", () => { | ||
| expect(parseBodySizeLimit("invalid")).toBe(1 * 1024 * 1024); | ||
| expect(parseBodySizeLimit("")).toBe(1 * 1024 * 1024); | ||
| expect(parseBodySizeLimit("10tb")).toBe(1 * 1024 * 1024); |
There was a problem hiding this comment.
Nice catch testing "10tb" as invalid. However, per my comment on the parser — Next.js does support terabytes (tb/TB). If you update the parser to match, this assertion would need to change accordingly.
| * Prevents unbounded request body buffering. | ||
| */ | ||
| var __MAX_ACTION_BODY_SIZE = 1 * 1024 * 1024; | ||
| var __MAX_ACTION_BODY_SIZE = ${JSON.stringify(bodySizeLimit)}; |
There was a problem hiding this comment.
JSON.stringify(bodySizeLimit) works correctly here since bodySizeLimit is always a number — it'll produce e.g. 10485760 in the generated code. Just noting for future readers that JSON.stringify on a number is equivalent to String() / template interpolation, so this is fine.
|
Review submitted. Here's a summary of my feedback on PR #338: Overall: The PR correctly implements the feature. The config parsing pipeline ( Key feedback:
|
2 participants
Adds support for reading experimental.serverActions.bodySizeLimit from next.config.ts and using it to configure the server action request body size limit. Currently, vinext hardcodes
__MAX_ACTION_BODY_SIZEto 1MB (1 * 1024 * 1024), ignoring any user configuration and causing 413 Payload Too Large errors when uploading files >1MB via server actions.Next.js respects this setting (docs), but vinext's generated dev server entry hardcodes the limit.
Problem
I faced an issue uploading files above 1MB in dev mode that drove this PR's creation. I had created a patch in my repo beforehand to correct the problem locally:
Test plan
Added 14 unit tests covering string parsing (mb/kb/gb/b), numeric passthrough, case insensitivity, fractional values, and default/invalid fallbacks
Added integration tests verifying
resolveNextConfig()end-to-end withexperimental.serverActions.bodySizeLimitAll existing tests continue to pass, oxlint was clean, typecheck had 4 existing errors prior to my changes.