fix(middleware): make an invalid root middleware export say how to fix it (depends on #2999) - #3011
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8690a44105
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const detail = looksLikeNext | ||
| ? `Found a named "middleware" export, which is the Next.js convention. ` + | ||
| `Veryfront expects a default export, and its middleware receives ` + | ||
| `(c, next) — a context carrying c.req, not the Request itself.` |
There was a problem hiding this comment.
Replace the em dash in the middleware error copy
When this invalid root middleware path is hit, the new user-facing TypeError includes the U+2014 em dash in (c, next) — a context...; the repo's public copy rules apply to error messages and require ASCII punctuation instead, so this text should use a comma or period before shipping.
AGENTS.md reference: AGENTS.md:L43-L55
Useful? React with 👍 / 👎.
55aca34 to
c5ea111
Compare
8690a44 to
9e4342a
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Score: 70/100
Requesting changes. The runtime change is low-risk and the diagnostic is much better, but the PR currently fails typechecking.
Blocking issue:
src/server/dev-server/middleware.test.ts:96and:111readerror.messagefrom the value returned byassertRejects, but in this wrapper the value is typed asunknown. Localdeno testtypechecking fails with TS18046.
Please narrow before reading the message, for example by asserting error instanceof TypeError, or use the project’s typed assertion pattern.
This PR is also stacked on lower open PRs and only shows CLA in the status rollup, so it is not merge-ready even after the local type error is fixed.
c5ea111 to
d223eeb
Compare
9e4342a to
cc313eb
Compare
d223eeb to
10b0811
Compare
cc313eb to
68ad259
Compare
|
Addressed. Both sites now narrow with That helper is declared All eight message assertions are unchanged, so the coverage that matters for this PR, the error copy itself, is intact: the file name, the Next.js convention, the Verification: |
10b0811 to
6490cad
Compare
68ad259 to
01e3d5e
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Follow-up after fixes: approving. The thrown-value handling now narrows the caught error before reading . Local verification: \running 2 tests from ./src/server/dev-server/middleware.test.ts
loadMiddlewareFile ...
fails closed for invalid production middleware ...
------- output -------
✘ [ERROR] Expected identifier but found end of file
../../../../app/middleware.ts:1:33:
1 │ export default function broken( {
╵ ^
22:30:19 SERVER ▲ [middleware] Failed to load middleware.ts: Build failed with 1 error:
../../../../app/middleware.ts:1:33: ERROR: Expected identifier but found end of file
----- output end -----
fails closed for invalid production middleware ... ok (17ms)
fails closed when production middleware has no valid default export ...
------- output -------
22:30:19 SERVER ▲ [middleware] Failed to load middleware.ts: Invalid middleware export in middleware.ts. Found a named "middleware" export, which is the Next.js convention. Veryfront expects a default export, and its middleware receives (c, next), where c is a context carrying c.req, not the Request itself.
Expected a default export that is a middleware function, or a non-empty array of them:
export default async function middleware(c, next) {
const response = await next();
return response;
}
See docs/guides/middleware.md.
----- output end -----
fails closed when production middleware has no valid default export ... ok (4ms)
fails closed when a production middleware array contains invalid entries ...
------- output -------
22:30:19 SERVER ▲ [middleware] Failed to load middleware.ts: Invalid middleware export in middleware.ts. Found export(s): default.
Expected a default export that is a middleware function, or a non-empty array of them:
export default async function middleware(c, next) {
const response = await next();
return response;
}
See docs/guides/middleware.md.
----- output end -----
fails closed when a production middleware array contains invalid entries ... ok (2ms)
preserves nonfatal development loading for invalid middleware ...
------- output -------
✘ [ERROR] Expected identifier but found end of file
../../../../app/middleware.ts:1:33:
1 │ export default function broken( {
╵ ^
22:30:19 SERVER ▲ [middleware] Failed to load middleware.ts: Build failed with 1 error:
../../../../app/middleware.ts:1:33: ERROR: Expected identifier but found end of file
----- output end -----
preserves nonfatal development loading for invalid middleware ... ok (1ms)
loadMiddlewareFile ... ok (25ms)
dev-server/middleware: actionable rejection ...
names the Next.js convention when a named middleware export is found ...
------- output -------
22:30:19 SERVER ▲ [middleware] Failed to load middleware.ts: Invalid middleware export in middleware.ts. Found a named "middleware" export, which is the Next.js convention. Veryfront expects a default export, and its middleware receives (c, next), where c is a context carrying c.req, not the Request itself.
Expected a default export that is a middleware function, or a non-empty array of them:
export default async function middleware(c, next) {
const response = await next();
return response;
}
See docs/guides/middleware.md.
----- output end -----
names the Next.js convention when a named middleware export is found ... ok (11ms)
lists the offending exports when the shape is merely wrong ...
------- output -------
22:30:19 SERVER ▲ [middleware] Failed to load middleware.ts: Invalid middleware export in middleware.ts. Found export(s): handler, other.
Expected a default export that is a middleware function, or a non-empty array of them:
export default async function middleware(c, next) {
const response = await next();
return response;
}
See docs/guides/middleware.md.
----- output end -----
lists the offending exports when the shape is merely wrong ... ok (2ms)
still accepts a valid default export ... ok (2ms)
still accepts an array of functions ... ok (3ms)
dev-server/middleware: actionable rejection ... ok (18ms)
ok | 2 passed (8 steps) | 0 failed (46ms). Score: 92/100. Next step: merge after base stack and checks are green.
|
Clean follow-up after the approval above: Score: 92/100. Verification:
Next step: merge after the base stack and refreshed checks are green. |
6490cad to
84b9305
Compare
01e3d5e to
5f3ef56
Compare
a829396 to
f4f212a
Compare
c14432c to
c1fac99
Compare
f4f212a to
4bc6a04
Compare
1f6c198 to
666f02f
Compare
7ea42f9 to
1beb02b
Compare
0d05b5d to
e4c7014
Compare
153b882 to
7811905
Compare
e4c7014 to
78cd493
Compare
7811905 to
e321644
Compare
78cd493 to
7c589d3
Compare
e321644 to
bb3bcde
Compare
1dbff3b to
b7ebd4b
Compare
bb3bcde to
a3ed383
Compare
b7ebd4b to
e4f1cce
Compare
e0f731f to
4eaf654
Compare
5874fea to
2aa1463
Compare
4eaf654 to
3e8f040
Compare
2aa1463 to
9e4d6c7
Compare
d962947 to
82d10a5
Compare
9e4d6c7 to
7bb9253
Compare
…x it
A root `middleware.ts` written to the Next.js convention (a named `middleware`
export) makes every single route 500, and the only diagnostic was:
Invalid middleware export: expected a function or non-empty array of functions
That names no file, no offending export, and no expected signature — so the
reasonable conclusion, and the one the reproducer reached, is that Veryfront has
no root middleware convention at all. It does, and it is documented; the error
just never pointed at it.
The message now names the file, lists what the module actually exported,
recognises the Next.js shape explicitly, shows the expected `(c, next)`
signature, and cites docs/guides/middleware.md.
Fixes bug 11 of the reproducer matrix.
7bb9253 to
8dad0c5
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Score: 93/100
Critical review result: this solves a real operational bug. A mis-shaped root middleware.ts takes down every route, so the old generic error forced users to infer the convention from source. The fix preserves fail-closed behavior, but now names the source file, distinguishes the named middleware export case, lists wrong exports for other shapes, and shows the expected default export signature.
Validation reviewed:
deno test --allow-all src/server/dev-server/middleware.test.tsdeno task lint:sanitizer-baselinedeno task lintdeno task typecheck
Recommended next step: merge after GitHub required checks complete green.
Summary
A root
middleware.tsusing a namedmiddlewareexport follows a common external convention, but Veryfront expects a default middleware function or a non-empty default array. Before this PR, strict middleware loading failed with a generic export-shape error that did not identify the file, the exported names, or the expected signature.This keeps invalid global middleware fail-closed, but makes the error actionable: it names
middleware.ts, detects the namedmiddlewareexport case, lists wrong exports otherwise, shows the(c, next)default-export shape, and points todocs/guides/middleware.md.Verification
deno test --allow-all src/server/dev-server/middleware.test.tsdeno task lint:sanitizer-baselinedeno task lintdeno task typecheckReview status
Score: 93/100. Recommended next step: merge after GitHub required checks finish green.