feat: make vinext fully compatible with clerk middleware#126
feat: make vinext fully compatible with clerk middleware#126nbardy wants to merge 1 commit intocloudflare:mainfrom
Conversation
nbardy
force-pushed
the
fix-clerk-middleware
branch
4 times, most recently
from
04daac0 to
7f5034d
Compare
nbardy
changed the title
…es to Cloudflare Worker ctx - Instantiates `NextFetchEvent` with `waitUntil` shim and passes to `middlewareFn` - Extracts `_waitUntilPromises` from the middleware result and attaches them to the `Response` object via `__vinextWaitUntil` - Updates the generated Cloudflare Worker `fetch` signature to include `ctx: ExecutionContext` - Iterates over `__vinextWaitUntil` on the final response and delegates to Cloudflare's native `ctx.waitUntil(promise)` - Enables background tasks (like telemetry and session sync in `@clerk/nextjs`) to survive the worker response lifecycle - Updates `vinext check` to explicitly report `@clerk/nextjs` as supported - Adds `app-router.test.ts` assertion to verify `waitUntil` injection
nbardy
force-pushed
the
fix-clerk-middleware
branch
from
7f5034d to
0cbbe57
Compare
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: waitUntil / NextFetchEvent support for Clerk middleware
The overall approach is sound — NextFetchEvent already existed in the codebase, and this PR wires it up to actually be instantiated, passed to middleware, and bubbled up to ctx.waitUntil(). The hidden-property pattern on Response is a reasonable pragmatic choice given the alternatives discussed in the PR description.
However, there are several issues that need to be addressed, most notably a dev/prod parity gap and lost waitUntil promises on early-return paths.
1. Dev/prod parity: prod-server.ts ignores waitUntilPromises
The Pages Router production server (prod-server.ts:750-751) calls runMiddleware() which now returns waitUntilPromises, but the result is silently discarded. Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as 'follow-ups' — fix them in the same PR."
The prod server runs in Node.js (not Workers), so there's no ctx.waitUntil(), but the promises should still be awaited (or Promise.allSettled'd) rather than silently swallowed. If Clerk schedules session sync via waitUntil, dropping those promises in prod mode means sessions won't sync during local production testing.
2. Early returns in app-dev-server.ts lose waitUntil promises
When middleware returns a redirect (line 1356) or a custom response (line 1376), the function returns mwResponse directly. The _waitUntilPromises array was populated on line 1336, but the Object.defineProperty that attaches them to the response only runs in the outer handler() function (line 1213). These early returns bypass that, so the promises are collected into _outerWaitUntilPromises but never attached to the response and never reach ctx.waitUntil().
Fix: attach __vinextWaitUntil to mwResponse before each early return:
if (_outerWaitUntilPromises.length > 0) {
Object.defineProperty(mwResponse, '__vinextWaitUntil', { value: _outerWaitUntilPromises, enumerable: false });
}
return mwResponse;This is a real bug — Clerk calls event.waitUntil() and then returns a redirect (the most common auth pattern: redirect unauthenticated users). The promises from that redirect path are lost.
| "@vercel/analytics": { status: "supported", detail: "analytics script injected client-side" }, | ||
| "next-intl": { status: "partial", detail: "works with middleware-based setup, some server component features may differ" }, | ||
| "@clerk/nextjs": { status: "unsupported", detail: "deep Next.js middleware integration not compatible" }, | ||
| "@clerk/nextjs": { status: "supported" }, |
There was a problem hiding this comment.
This is premature. The PR description explicitly states a dependency on an unmerged Clerk PR (clerk/javascript#7954) for ESM compatibility. Until that ships, @clerk/nextjs won't load in Vite/Workers. Marking it as "supported" will mislead users running vinext check.
Consider "partial" with a detail string, or keep this change gated until the Clerk side lands:
| "@clerk/nextjs": { status: "supported" }, | |
| "@clerk/nextjs": { status: "partial", detail: "waitUntil supported; requires @clerk/nextjs ESM fix (clerk/javascript#7954)" }, |
| } | ||
|
|
||
| export default async function handler(request) { | ||
| let _waitUntilPromises = []; |
There was a problem hiding this comment.
Minor: the binding itself is never reassigned — only the array contents are mutated. const is more appropriate.
| let _waitUntilPromises = []; | |
| const _waitUntilPromises = []; |
| // Delegate to RSC handler (which decodes + normalizes the pathname itself) | ||
| const result = await rscHandler(request); | ||
|
|
||
| // If the middleware registered any waitUntil promises, hand them off to the runtime |
There was a problem hiding this comment.
This check runs before the instanceof Response check on line 62. If result is a Response (the common case), this works — the in operator finds the non-enumerable property, promises are handed off, then the Response is returned below.
A brief comment explaining why this precedes the instanceof check would help future readers:
| // If the middleware registered any waitUntil promises, hand them off to the runtime | |
| // Extract waitUntil promises BEFORE the instanceof check — the property is | |
| // non-enumerable on the Response and we need to hand it off to ctx before returning. | |
| if (result && typeof result === "object" && "__vinextWaitUntil" in result && Array.isArray(result.__vinextWaitUntil)) { |
|
|
||
|
|
||
| // Bubble up any background tasks attached by the app-dev-server layer | ||
| if ("__vinextWaitUntil" in response) { |
There was a problem hiding this comment.
The guard here is less robust than the equivalent in app-router-entry.ts (line 56), which does result && typeof result === 'object' && '__vinextWaitUntil' in result && Array.isArray(...). Consider aligning:
| if ("__vinextWaitUntil" in response) { | |
| if (response && typeof response === "object" && "__vinextWaitUntil" in response && Array.isArray((response as any).__vinextWaitUntil)) { |
Also, nit: there's an extra blank line above this comment (line 681).
| this._waitUntilPromises.push(promise); | ||
| } | ||
|
|
||
| get waitUntilPromises(): Promise<unknown>[] { |
There was a problem hiding this comment.
Exposing the private array directly means callers can mutate it (push, splice, etc.). This is fine for the current internal-only usage, but worth noting the tradeoff. If this ever becomes part of a public API surface, consider returning a copy ([...this._waitUntilPromises]). Not blocking.
|
|
||
| it("middleware receives event with waitUntil (for Clerk compat)", async () => { | ||
| const res = await fetch(`${baseUrl}/middleware-event`); | ||
| expect(res.status).toBe(200); |
There was a problem hiding this comment.
This test verifies that event.waitUntil exists and is callable, which is good for basic coverage. But it doesn't verify the bubble-up behavior — that the promises actually end up in MiddlewareResult.waitUntilPromises and eventually reach ctx.waitUntil().
Consider adding a unit test that calls runMiddleware() from middleware.ts directly and asserts that result.waitUntilPromises contains the registered promises. That would cover the most critical behavior in this PR — the plumbing — without needing an e2e Worker setup.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
@southpolesteve Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
2 participants
Description
This PR makes
vinextfully compatible with@clerk/nextjsby implementing theNextFetchEvent.waitUntilbackground task API across the middleware and Worker layers.What changed
NextFetchEventto middleware: Construct the event with awaitUntilarray and pass it as the second argument tomiddlewareFn. This is the standard Next.js middleware signature — Clerk usesevent.waitUntil()to schedule session sync and telemetry as background tasks.waitUntilPromisesfrom the middleware result and attach them to theResponseobject via a non-enumerable__vinextWaitUntilproperty so they survive the routing pipeline.ctx: ExecutionContextto the generated Workerfetchsignature, collect promises from bothrunMiddlewareand__vinextWaitUntil, and delegate to Cloudflare's nativectx.waitUntil(). This ensures background work survives the serverless response lifecycle.vinext checknow reports@clerk/nextjsas supported.app-router.test.tscoverage verifyingevent.waitUntilis injected correctly.Architectural note: from workaround to native API support
The original framing of this work was "making Clerk not crash." The paired Clerk PR changes that picture: once Clerk's ESM imports are fixed,
@clerk/nextjsloads cleanly in Vite/Workers. What vinext needs to do is provide the correct API surface — specifically, a realNextFetchEventwith a functioningwaitUntil(). This is not a shim to paper over bugs; it is the standard Next.js middleware contract that Clerk (and any other middleware library) legitimately depends on.Architectural Decision: The 'Bubble Up' Pattern
Background promises need to travel from the middleware layer up to the Cloudflare Worker's
ctx.waitUntil(). This PR uses a hidden property (Object.defineProperty(response, '__vinextWaitUntil', ...)).Alternatives considered:
ReadableStreamis returned; the ALS scope can tear down before background tasks are safely handed off to Cloudflare.{ response, tasks }): Changes the core router return type fromResponseto a tuple — massive blast radius across every caching, proxy, and error boundary layer.WeakMapkeyed toRequest: Fragile because the framework frequently clones requests when manipulating headers and URLs, which drops the reference.Attaching promises directly to the
Responseas a non-enumerable property lets them travel safely up the execution chain, survive streaming, and require zero signature changes across the broader codebase.Clerk API contact points
NextFetchEvent+event.waitUntil()NextResponse.next({ request: { headers } })/x-middleware-request-*req.cookies/res.cookies.set()req.nextUrl/NextURLNextResponse.redirect()early return@clerk/nextjspackage