fix(build): don't crash on benign floating asset-import errors (#1510)

[james-elicx]

Fixes #1510 — the react-version and twoslash test apps crashed the server (exit 7/1) when a floating (un-awaited) import() of an asset failed to resolve: the uncaughtException/unhandledRejection backstop re-threw it and took down the process. The backstop now swallows benign asset-import resolution errors (matching Next.js's non-crashing stance) so the build/serve completes; genuine JS/TS module errors still surface. Tests added in socket-error-backstop.test.ts. Closes #1510

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@1846

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

commit: bfdf755

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[james-elicx]

/bigbonk review for issues

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[ask-bonk]

Review: fix(build): don't crash on benign floating asset-import errors (#1510)

Reviewed the backstop changes and tests against real Node error shapes and Next.js's documented behavior. The fix is correct and well-targeted. I verified the core claims empirically and against the Next.js source.

Verified correct

• Next.js parity is real. The exact fixture this targets is test/e2e/react-version/pages/api/pages-api-edge-url-dep.js, whose comment literally reads // Adding URL dependency to edge api, it shouldn't break the build and does a floating import(new URL('./style.css', import.meta.url).href). So treating these as benign matches Next.js intent.
• Error shapes match. I reproduced both branches on Node:
  • On-disk asset → ERR_UNKNOWN_FILE_EXTENSION with url: undefined and the ext in the message (Unknown file extension ".css" for /...). The regex /extension\s+"([^"]+)"/ + toLowerCase() allow-list handles this, including uppercase exts and paths containing stray quotes.
  • Missing asset → ERR_MODULE_NOT_FOUND with url populated. The url-extension check handles this, including ?v= query suffixes (covered by a test).
• Allow-list scoping is sound. Excluding .js/.mjs/.ts/extensionless means genuine missing-module bugs still crash. In a Vite server bundle, CSS/asset imports are build-time transforms; a runtime import() of a .css reaching Node can only ever fail benignly, so the absorption is safe.
• Handler precedence is deliberate and documented. absorbBenignAssetImport runs before the VINEXT_PRERENDER re-throw, so benign asset imports are swallowed even during prerender (intended — "so the build doesn't fail"). This is a documented, narrow exception to the otherwise re-throw-everything prerender stance.
• vp check passes (format + lint + types) on both files; the test file passes (8 tests).

Non-blocking observations

1. .wasm in the allow-list is effectively dead for the on-disk case. Node loads .wasm natively, so a present-on-disk wasm asset throws a WebAssembly CompileError (no code), not ERR_UNKNOWN_FILE_EXTENSION — it won't be matched by the extension branch. A missing .wasm still matches via the ERR_MODULE_NOT_FOUND url branch, so the entry isn't entirely useless, but the on-disk path won't trigger. Minor; worth a one-line comment to avoid future confusion. Not a correctness issue.

2. Installed-handler behavior is untested. The new tests cover the pure predicates (isBenignAssetImportError, peerDisconnectCode) and the test-runner install skip, which is the right unit boundary. But the interaction — specifically that a benign asset import is absorbed even when VINEXT_PRERENDER=1 (the precedence that makes this fix work during build) — has no direct test. Since the listener is gated off under Vitest, you'd need to invoke the handler body directly (e.g. extract an absorbBenignAssetImport-style pure function that takes the env state, or test the registered callback). Consider adding one test asserting "benign asset import is absorbed; VINEXT_PRERENDER re-throw does not pre-empt it" to lock in the ordering against future refactors.

3. Comment accuracy nit (line 99-100): the doc says ERR_UNKNOWN_FILE_EXTENSION (asset present) / ERR_MODULE_NOT_FOUND (asset not co-located) — accurate, but note .json and .wasm are special-cased by Node (ERR_IMPORT_ATTRIBUTE_MISSING / WebAssembly errors respectively) and won't hit either branch when present. The allow-list correctly omits .json; only the .wasm mention is slightly aspirational.

Conclusion

The change is correct, conservative, and faithful to Next.js behavior. The only thing I'd genuinely push for before merge is observation #2 — a test that pins the benign-absorb-before-prerender-rethrow ordering, since that precedence is the load-bearing part of the fix and is currently only asserted by the predicate tests, not the handler. The .wasm/comment items are cosmetic.

[ask-bonk]

Review posted to PR #1846.

Summary

I reviewed the PR's changes to socket-error-backstop.ts and the new tests, verifying claims empirically against real Node error shapes and the Next.js source.

Verdict: the fix is correct and well-targeted.

Key verifications:

• Next.js parity confirmed — the exact fixture (test/e2e/react-version/pages/api/pages-api-edge-url-dep.js) does a floating import(new URL('./style.css', import.meta.url).href) with the comment "it shouldn't break the build", so treating these as benign matches Next.js intent.
• Both error branches match real Node behavior — ERR_UNKNOWN_FILE_EXTENSION (ext in message, no url) for on-disk assets, ERR_MODULE_NOT_FOUND (url populated) for missing assets. Verified the regex and url-extension matching handle uppercase exts, ?v= query suffixes, and stray quotes in paths.
• Allow-list scoping is safe — excluding JS/TS extensions means genuine module bugs still crash; runtime import() of CSS in a Vite bundle can only fail benignly.
• vp check clean; tests pass.

Non-blocking items flagged:

1. .wasm allow-list entry is effectively dead for the on-disk case (Node loads wasm natively → CompileError, not ERR_UNKNOWN_FILE_EXTENSION).
2. Main suggestion: the installed-handler precedence (benign-asset absorbed before the VINEXT_PRERENDER re-throw — the load-bearing part of the fix during build) is untested. Only the pure predicates are covered. Recommended adding one test pinning that ordering.
3. Minor comment-accuracy nit re: .json/.wasm Node special-casing.

github run