fix: add tests verifying strings thrown in Server Components are not swallowed

[Divkix]

Fixes #810

Summary

• vinext's rscOnError already correctly handles strings thrown in Server Components — no early-return swallow bug exists (unlike the Next.js bug fixed in vercel/next.js@b9ca95c62)
• Added fixture page (throw-string-test) that throws a string in a Server Component with an error boundary
• Added integration test verifying the error boundary renders with the string message
• Added runtime test confirming rscOnError returns a valid digest hash for string throws in production
• Added static test confirming generated code has no typeof thrownValue === 'string' early-return path

Test plan

• Integration test: error boundary catches string thrown in Server Component and displays the message
• Runtime test: rscOnError processes string throws through normal error path (produces digest in production)
• Static test: generated code free of the string early-return anti-pattern

Upstream: vercel/next.js@b9ca95c

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@950

commit: 295a490

[james-elicx]

/bigbonk review

[ask-bonk]

Review

The integration test and fixture are good additions. However, the two unit tests added to the runtime behavior describe block have issues that need fixing before merge.

Summary of issues

1. Duplicate rscOnError construction — The "does not swallow strings" test reconstructs rscOnError via new Function identically to what beforeAll already does, but with NODE_ENV=production. The existing rscOnError from beforeAll is in dev mode, so testing production behavior is valid, but the approach should use the already-extracted digestFn/onErrorFn strings without duplicating the Function construction ceremony. More importantly, the test creates a second instance but the beforeAll already provides one — the test should just create a production variant cleanly.

2. Static test asserts on implementation details that could break — The "does not have an early return for string thrown values" test asserts expect(onErrorFn).toContain("error instanceof Error") and expect(onErrorFn).toContain("String(error)"). These are asserting on incidental implementation details of the generated code that could change for valid reasons (e.g., refactoring the error handler to use a helper function). The negative assertion (not toContain "typeof thrownValue === 'string'") is the valuable part — it guards against the specific anti-pattern from the Next.js bug. The positive assertions add brittleness without proportional value.

3. Integration test could be more precise — The test checks html contains the thrown string, but doesn't verify it appears inside the error boundary's data-testid="string-error-message" element. The string could appear in a React error overlay or console dump rather than the error boundary. Using the existing textContentByTestId helper would make this more robust.

See inline comments for details.

[ask-bonk]

This is a minor point, but the runtime test duplicates the new Function(...) construction that beforeAll already performs. The only difference is NODE_ENV: "production" vs "development". Consider creating the production variant alongside the dev one in beforeAll (or a shared helper) instead of duplicating the ceremony here.

That said, this test is the most valuable one in this PR — it directly verifies the behavior. The assertion structure is good: it checks the return value is defined, is a string, is non-empty, and is content-dependent. This is a solid test.

[ask-bonk]

The negative assertion on line 3740 is the valuable guard — it prevents the specific anti-pattern from the Next.js bug. But these two positive assertions (toContain("error instanceof Error") and toContain("String(error)")) are asserting on incidental implementation details of the generated code. If someone refactors rscOnError to use a helper like getErrorMessage(error) internally, these assertions break even though the behavior is correct.

I'd recommend removing the two positive toContain assertions and keeping only the negative one. The runtime test above already proves strings are handled correctly — this static test's job is just to ensure the known-bad pattern isn't reintroduced.

Suggested change

	expect(onErrorFn).toContain("String(error)");
	expect(onErrorFn).not.toContain("typeof thrownValue === 'string'");
	});

[ask-bonk]

This checks that the thrown string appears somewhere in the HTML, but it could be in a React error overlay or devtools dump rather than inside the error boundary. The file already has a textContentByTestId helper — using it here would confirm the string actually reached the error boundary component:

Suggested change

	expect(html).toContain("this is a test string thrown in a server component");
	expect(textContentByTestId(html, "string-error-message")).toBe(
	"this is a test string thrown in a server component",
	);

This is more precise and exercises the same code path the fixture was designed to test.

[ask-bonk]

Review submitted. Here's a summary of the feedback I posted:

Requested changes on PR #950 with 3 inline comments:

1. Runtime test (line 3734): The new Function(...) construction is duplicated from beforeAll — the only difference is NODE_ENV. Suggested consolidating. Noted this is still the most valuable test in the PR since it directly proves strings aren't swallowed.

2. Static test (line 3743): The positive assertions checking for "error instanceof Error" and "String(error)" in the generated code are brittle implementation-detail checks. Suggested keeping only the negative assertion (not toContain "typeof thrownValue === 'string'") which is the actual anti-pattern guard, and provided a concrete suggestion.

3. Integration test (line 821): The test checks the thrown string appears "somewhere in the HTML" but should use the existing textContentByTestId helper to verify it actually rendered inside the error boundary's data-testid="string-error-message" element. Provided a suggestion with the more precise assertion.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

All three comments from the previous review have been addressed cleanly:

1. prodRscOnError moved into beforeAll — no more duplicated new Function construction. The factory is called once with both dev and production NODE_ENV values.
2. Brittle positive assertions removed — the static test now only has the negative anti-pattern guard (not.toContain("typeof thrownValue === 'string'")).
3. Integration test uses textContentByTestId — confirms the string actually rendered inside the error boundary element, not just somewhere in the HTML.

CI is all green. The test coverage is well-structured: integration test proves end-to-end behavior, runtime test proves rscOnError handles strings correctly in production mode, and static test guards against the specific Next.js anti-pattern being reintroduced. Good to merge.

[ask-bonk]

Approved PR #950. All three comments from the previous review were addressed in the second commit:

1. prodRscOnError moved into beforeAll — no duplicate new Function construction
2. Brittle positive assertions removed — only the negative anti-pattern guard remains
3. Integration test uses textContentByTestId for precise error boundary verification

CI is fully green. The test coverage is well-layered (integration, runtime, static) and the fixtures are clean. Good to merge.

github run