fix(middleware): shim __filename and __dirname for user proxy/middleware (#1546)#1650
fix(middleware): shim __filename and __dirname for user proxy/middleware (#1546)#1650james-elicx wants to merge 1 commit into
Conversation
…are (#1546) User middleware/proxy files referenced `__filename` and `__dirname` throw `ReferenceError` because vinext bundles them as ESM. The middleware runtime catches the error and returns a 500, which surfaces as an empty body to any client request that touches the offending branch (in `proxy-nfc-traced` the client follows the `/home → /` redirect into the crashing fall-through). Mirror Next.js's webpack template by injecting `const __filename` / `__dirname` shims at build time for the resolved middleware/proxy file, using the same Vite-plugin pattern already used for `next.config.ts`. Skip the corresponding `vinext check` warning for that file too.
|
/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.
/bigbonk review
Clean, well-scoped fix. The plugin implementation is correct, the check.ts exclusion is properly placed, and the test strategy (structural guards on bundled output + e2e) is thoughtful. A few observations and one minor concern:
What's good
-
The
continueplacement in check.ts is correct. It sits after the ViewTransition check (line 644) but before the CJS globals scan (line 657), so middleware/proxy files still get checked for ViewTransition imports — only the__dirname/__filenamewarning is suppressed. This is the right scoping. -
Dev/prod parity is not a concern here. The plugin is a Vite
transformhook that runs during bundling, so it applies uniformly to dev, prod, and Cloudflare Workers builds. The middleware path comes from the samefindMiddlewareFilecall in all cases. -
The lazy getter pattern is correct.
middlewarePathis set during theconfighook (index.ts:1097), andrefreshTarget()runs inbuildStart, which fires afterconfig— so the closure always reads a populated value. -
Test quality is strong. The explicit acknowledgment that vitest masks the production failure mode, combined with the structural bundle assertions (
extractProxyRegion+ regex match on the emittedconst __filename), is the right way to guard this regression. The sanity check thatconsole.log(__filename)is still present prevents the structural guards from becoming vacuously true if a future Rolldown change elides the reference.
Observations
-
isShimmedMiddlewareFileregex is broader thanfindMiddlewareFile. The regex accepts.cjs,.mjs,.cts,.mtsextensions via[cm]?[jt]sx?, butfindMiddlewareFileonly searches for extensions fromDEFAULT_PAGE_EXTENSIONS(tsx, ts, jsx, js) unless the user has custompageExtensions. This is a harmless false-negative suppression (worst case: we skip a CJS warning for a.mjsmiddleware file that doesn't actually exist), but worth noting for future readers. IfpageExtensionsis ever expanded to include.mts/.ctsvariants upstream, this regex already handles it. -
map: nullis acceptable. Both this plugin and the existingcjsGlobalsInjectorPluginfor next.config.ts returnmap: null. Since the preamble is only 2 lines, error stack offsets shift by +2 lines in the middleware file. In practice, middleware files are small and the offset is negligible. This is consistent with the existing pattern. -
Consolidation with
cjsGlobalsInjectorPlugin. The PR description notes the similarity. The two plugins have different scopes — the next-config version also handlesrequire,module,exports, andmodule.exportsreassignment, which middleware doesn't need. Keeping them separate is the right call for now; the middleware version is a strict subset. A future refactor could extract the shared__filename/__dirnameinjection logic into a helper, but that's not blocking. -
safeRealpathis duplicated. This small utility now exists in bothconfig/next-config.tsandplugins/middleware-cjs-globals.ts. Not a blocking concern for a 3-line function, but if a third copy appears it should be extracted.
| * Issue: https://github.com/cloudflare/vinext/issues/1546 | ||
| */ | ||
|
|
||
| const CJS_GLOBAL_SCAN_REGEX = /\b(?:__filename|__dirname)\b/; |
There was a problem hiding this comment.
Nit: The CJS_GLOBAL_SCAN_REGEX here uses a non-global regex (no g flag), which is correct for the .test() usage on line 77 — you only need to know whether a match exists, not iterate. Good. The check.ts version uses the g flag because it iterates with .exec(). Worth noting for anyone comparing the two.
|
|
||
| return { | ||
| code: preamble + code, | ||
| map: null, |
There was a problem hiding this comment.
Both this plugin and cjsGlobalsInjectorPlugin in next-config.ts return map: null. For a 2-line preamble this is fine in practice, but if the preamble ever grows (or if someone reports confusing stack traces in middleware), generating a proper source map with MagicString would be straightforward. Not blocking.
| */ | ||
| function isShimmedMiddlewareFile(relPath: string): boolean { | ||
| const normalized = relPath.split(path.sep).join("/"); | ||
| return /^(?:src\/)?(?:middleware|proxy)\.[cm]?[jt]sx?$/.test(normalized); |
There was a problem hiding this comment.
The regex [cm]?[jt]sx? accepts extensions like .mts, .cts, .mjs, .cjs that findMiddlewareFile does not currently search for (it uses DEFAULT_PAGE_EXTENSIONS = tsx, ts, jsx, js). This is a safe over-match — suppressing a warning for a file that the shim plugin would also target if found — but if precision matters, you could tighten this to \.[jt]sx?$ to match the default extensions exactly. Fine either way.
| throw new Error("Could not find matching endregion for proxy.ts"); | ||
| } | ||
| return remainder.slice(0, endRelative); | ||
| } |
There was a problem hiding this comment.
extractProxyRegion relies on Rolldown emitting //#region markers. This is a Rolldown implementation detail that could change across versions. If a future Rolldown update removes or changes the region comment format, these structural tests will throw rather than silently pass — which is the right failure mode (explicit breakage, not false green). Good defensive design.
|
Both the summary review and inline comments have been posted. Here's a summary of the review: Review posted on PR #1650 with 4 inline comments. Verdict: Clean, well-scoped fix. No blocking issues found. Key findings:
|
1 participant
Summary
proxy.ts/middleware.tsfiles that reference__filenameor__dirnamethrewReferenceErrorat runtime because vinext bundles them as ESM. The middleware runtime caught the error and returned a 500, which the upstreamproxy-nfc-traceddeploy test sees as an empty body when following the/home → /redirect into the crashing branch.createMiddlewareCjsGlobalsPlugin) that injectsconst __filename/const __dirnameat the top of the resolved middleware/proxy module's source, matching Next.js's webpack template behaviour. This is the same pattern already used bycjsGlobalsInjectorPluginfornext.config.ts.vinext checkwarning for the shimmed middleware/proxy file.Fixes #1546.
Test plan
tests/proxy-nfc-traced.test.tsbuilds a minimal app +proxy.tsfixture, then asserts both that the bundled output contains the injected__filename/__dirnameconst declarations (the structural guard — vitest itself provides__filename, masking the production failure) and that the HTTP responses for/home(with redirect follow) and/renderhello world.pnpm test tests/proxy-nfc-traced.test.ts— 6/6 passpnpm test tests/app-router.test.ts— 331/331 passpnpm test tests/prerender.test.ts— 77/77 passpnpm test tests/pages-router.test.ts— 252/252 passpnpm test tests/middleware-runtime.test.ts tests/middleware-runtime-trailing-slash.test.ts tests/middleware-server-only.test.ts tests/shims.test.ts— 1016/1016 passpnpm test tests/check.test.ts— 99/99 passpnpm run check— clean (format + lint + types)vinext build --prerender-all && vinext startagainst the upstream fixture now serves<p>hello world</p>forGET /home(previously returnedInternal Server Error)