feat: make vinext fully compatible with clerk middleware#126
feat: make vinext fully compatible with clerk middleware#126nbardy wants to merge 3 commits 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.
packages/vinext/src/check.ts
Outdated
| "@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)) { |
packages/vinext/src/deploy.ts
Outdated
|
|
||
|
|
||
| // 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. |
# Conflicts: # packages/vinext/src/server/app-dev-server.ts # packages/vinext/src/server/middleware.ts
- Fix lost waitUntil promises on redirect/custom response early returns - Settle waitUntil promises in prod-server (Node.js) instead of dropping - Mark @clerk/nextjs as "partial" until ESM fix lands (clerk/javascript#7954) - Align __vinextWaitUntil guard in deploy.ts with app-router-entry.ts - Use const for non-reassigned _waitUntilPromises array - Add comment explaining waitUntil check ordering in app-router-entry - Add unit test verifying runMiddleware bubbles up waitUntil promises Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/bigbonk review |
|
@ask-bok I fixed it |
Note on Clerk dependencyThis PR on its own provides general
Without the Clerk PR, The waitUntil infrastructure is still independently useful for custom middleware and other libraries that use |
|
/bigbonk review |
There was a problem hiding this comment.
Review: waitUntil / NextFetchEvent plumbing for Clerk middleware
The overall architecture is sound. The hidden-property bubble-up pattern is a pragmatic choice well-justified by the alternatives analysis in the PR description. The implementation is thorough across the App Router dev server, the generated virtual middleware module, and the Cloudflare Worker entries.
Good improvements since the previous review: the early-return paths in app-dev-server.ts now correctly attach __vinextWaitUntil before returning, and check.ts accurately reports "partial" status. The new shims.test.ts test that calls runMiddleware() directly and asserts on result.waitUntilPromises is exactly the right kind of test.
Bug: prod-server.ts early returns drop waitUntilPromises
The Promise.allSettled call (line 837) only runs in the result.continue === true path. When middleware returns continue: false — i.e., a redirect (line 780-786) or a custom response (line 788-803) — the function returns early before reaching the Promise.allSettled call. This means Clerk's session sync promises are silently dropped in the most common auth pattern: redirecting unauthenticated users.
Compare with deploy.ts line 558, which correctly places the waitUntilPromises extraction before the !result.continue check. The prod-server should follow the same pattern — move the Promise.allSettled block above the if (!result.continue) check.
Minor: error catch path in app-dev-server.ts loses promises
If middleware calls event.waitUntil(p) and then throws, the catch block (line 1456-1458) returns a bare new Response("Internal Server Error", { status: 500 }) without attaching __vinextWaitUntil. Edge case and not blocking, but noted for completeness.
Positive notes
- The guard in
app-router-entry.ts(result && typeof result === 'object' && '__vinextWaitUntil' in result && Array.isArray(...)) is thorough. Theinoperator correctly detects non-enumerable properties. - The Pages Router worker entry in
deploy.tscorrectly placeswaitUntilPromisesextraction before early returns (line 558), so both redirect and custom response paths are covered. - The
check.tschange to"partial"with the dependency note accurately reflects the current state.
| // Await any background promises registered via event.waitUntil() during middleware. | ||
| // In prod-server (Node.js) there's no Workers ctx.waitUntil(), so we settle them | ||
| // here to avoid silently dropping work (e.g. Clerk session sync). | ||
| if (result.waitUntilPromises && result.waitUntilPromises.length > 0) { | ||
| Promise.allSettled(result.waitUntilPromises).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
Bug: this Promise.allSettled block is placed after the !result.continue early returns (lines 780-803, not in this diff). When middleware returns a redirect or custom response (continue: false), the function returns before reaching this code, so waitUntilPromises are silently dropped.
This is the most common Clerk auth pattern — redirect unauthenticated users. Compare with deploy.ts line 558, which correctly extracts waitUntilPromises before the !result.continue check.
Fix: move this block above the if (!result.continue) guard (around line 779), so promises are settled regardless of the middleware result path:
const result = await runMiddleware(webRequest);
// Settle waitUntil promises immediately — in Node.js there's no ctx.waitUntil().
if (result.waitUntilPromises?.length) {
Promise.allSettled(result.waitUntilPromises).catch(() => {});
}
if (!result.continue) {
// ... existing redirect/response handling| // Bubble up waitUntil promises (e.g. Clerk telemetry/session sync) | ||
| if (result.waitUntilPromises?.length) { | ||
| for (const p of result.waitUntilPromises) { | ||
| ctx.waitUntil(p); | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct placement — extracting waitUntilPromises before the !result.continue check ensures promises survive all middleware result paths (redirect, custom response, continue). This is the pattern that prod-server.ts should follow too.
| response.headers.set(h.key, h.value); | ||
| } | ||
| } | ||
|
|
||
| // Expose waitUntil promises to the runtime (like Cloudflare Workers) | ||
| if (response && _waitUntilPromises && _waitUntilPromises.length > 0) { | ||
| Object.defineProperty(response, "__vinextWaitUntil", { value: _waitUntilPromises, enumerable: false }); |
There was a problem hiding this comment.
This only runs for responses that flow through the normal _handleRequest return path. The early-return cases (redirect on line 1429, custom response on line 1452) now also attach __vinextWaitUntil — good, that was a gap in the initial version of this PR.
| } else { | ||
| // Check for redirect | ||
| if (mwResponse.status >= 300 && mwResponse.status < 400) { | ||
| // Attach waitUntil promises before early return so they reach ctx.waitUntil(). | ||
| // This is the most common Clerk auth pattern: redirect unauthenticated users. | ||
| if (_outerWaitUntilPromises.length > 0) { | ||
| Object.defineProperty(mwResponse, "__vinextWaitUntil", { value: _outerWaitUntilPromises, enumerable: false }); | ||
| } | ||
| return mwResponse; | ||
| } | ||
| // Check for rewrite |
There was a problem hiding this comment.
Good — this covers the most critical early-return path (Clerk auth redirect). The Object.defineProperty with enumerable: false ensures the property doesn't leak into JSON serialization or header iteration.
| // 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)) { | ||
| for (const p of result.__vinextWaitUntil) { | ||
| ctx.waitUntil(p); | ||
| } | ||
| } |
There was a problem hiding this comment.
The guard is thorough and correctly placed before the instanceof Response check. The in operator detects non-enumerable properties, so this works with the Object.defineProperty(..., { enumerable: false }) pattern used upstream. Good.
| // If the middleware registered any waitUntil promises, we need to bubble them up | ||
| // to the runtime (like Cloudflare Workers) so they aren't cancelled. | ||
| if (mwEvent.waitUntilPromises && mwEvent.waitUntilPromises.length > 0) { | ||
| _outerWaitUntilPromises.push(...mwEvent.waitUntilPromises); | ||
| } |
There was a problem hiding this comment.
Nit: the && mwEvent.waitUntilPromises.length > 0 check is unnecessary — spreading an empty array is a no-op and the downstream checks all guard on length. Not worth changing, just noting the redundancy.
| it("runMiddleware bubbles up waitUntil promises in result", async () => { | ||
| const { runMiddleware } = await import( | ||
| "../packages/vinext/src/server/middleware.js" | ||
| ); | ||
|
|
||
| let capturedPromise: Promise<unknown> | null = null; | ||
| const mockServer = { | ||
| ssrLoadModule: async () => ({ | ||
| middleware: (_req: Request, event: { waitUntil: (p: Promise<unknown>) => void }) => { | ||
| const p = Promise.resolve("background-work"); | ||
| capturedPromise = p; | ||
| event.waitUntil(p); | ||
| return Response.redirect("http://localhost/login", 307); | ||
| }, | ||
| config: { matcher: ["/protected"] }, | ||
| }), | ||
| }; | ||
|
|
||
| const request = new Request("http://localhost/protected"); | ||
| const result = await runMiddleware(mockServer as any, "/tmp/middleware.ts", request); | ||
|
|
||
| // The most critical behavior: waitUntil promises must appear in the result | ||
| // so the runtime (e.g. Cloudflare Workers ctx.waitUntil) can keep them alive. | ||
| expect(result.waitUntilPromises).toBeDefined(); | ||
| expect(result.waitUntilPromises.length).toBe(1); | ||
| expect(result.waitUntilPromises[0]).toBe(capturedPromise); | ||
| }); |
There was a problem hiding this comment.
Good test — calls runMiddleware() directly, has the middleware call event.waitUntil(p) then return a redirect, and asserts the promise appears in result.waitUntilPromises. This covers the exact Clerk auth redirect pattern end-to-end through the middleware.ts plumbing.
One thing this doesn't cover: the bubble-up to __vinextWaitUntil on the Response object (the app-dev-server.ts layer). That would require a more involved integration test, but the current coverage is reasonable for this PR.
|
The review was posted successfully. Here's a summary of the review: Review Summary for PR #126Architecture: Sound. The hidden-property ( One Bug Found
Minor IssueError catch path in Positive Notes
|
3 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