fix: bypass the request when the server.proxy is specified#3816
fix: bypass the request when the server.proxy is specified#3816Hajime-san wants to merge 5 commits into
server.proxy is specified#3816Conversation
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Approach looks right — matches Vite's own startsWith/^-prefix semantics, and precomputing the matcher in configureServer is nicer than rebuilding regexes per request. Left a few inline notes; once those are addressed I think this is good to go.
| const IGNORE_URLS = new RegExp( | ||
| `^(${base})?/(@(vite|fs|id)|\\.vite)/`, | ||
| ); | ||
| // build proxy url list matcher beofre the request is coming |
There was a problem hiding this comment.
Typo: beofre → before. Also worth rewording — the current phrasing is a bit hard to parse.
| // build proxy url list matcher beofre the request is coming | |
| // Precompute the proxy URL matcher; proxy config is fixed at server start. |
| * Handling the user config of proxy | ||
| * https://vite.dev/config/server-options#server-proxy | ||
| */ | ||
| function createProxyUrlMatcher( |
There was a problem hiding this comment.
One behavioral gap worth flagging: Vite's proxy options support a bypass(req, res, options) callback per entry that can return false to opt out of proxying for a given request. With this matcher, such requests would still be short-circuited away from Fresh and then fall through Vite's proxy without being proxied, ending up as 404s. Probably uncommon in practice — fine to leave for a follow-up, but a code comment noting the limitation would help future readers.
| { | ||
| const res = await fetch(`${address}/api3/pong?z=3`); | ||
| expect(res.status).toEqual(200); | ||
| expect(await res.text()).toEqual("api3"); | ||
| } |
There was a problem hiding this comment.
Consider adding a negative-case assertion so we prove the bypass is selective — e.g. that / still hits the Fresh route. Without it, a regression that bypasses every request would still pass these checks.
| { | |
| const res = await fetch(`${address}/api3/pong?z=3`); | |
| expect(res.status).toEqual(200); | |
| expect(await res.text()).toEqual("api3"); | |
| } | |
| { | |
| const res = await fetch(`${address}/api3/pong?z=3`); | |
| expect(res.status).toEqual(200); | |
| expect(await res.text()).toEqual("api3"); | |
| } | |
| { | |
| const res = await fetch(`${address}/`); | |
| expect(res.status).toEqual(200); | |
| expect(await res.text()).toEqual("ok"); | |
| } |
There was a problem hiding this comment.
Exactly, thanks!
- Fix typo and rephrase comment ("build proxy url list matcher beofre")
- Add JSDoc note on the bypass() callback limitation
- Add negative-case assertion to prove Fresh routes are still reachable
2 participants
server.proxynot working with@fresh/plugin-vite#3814