feat: Make URL rewriting possible#3812
Conversation
bartlomieju
left a comment
There was a problem hiding this comment.
Nice feature, and well-scoped for a first contribution. The security-relevant pieces (same-origin enforcement, path normalization on every dispatch, loop cap, body-used guard) all look right, and the test coverage is thorough.
A few things to consider, mostly polish — see inline comments. The two that matter most:
- Behavior of string targets when
basePathis configured — currentlynew URL("/new", ctx.url)will drop the basePath segment, soctx.rewrite("/new")won't match routes registered under the basePath. The basePath test sidesteps this by passing a fullURL. Worth deciding whether string targets should be auto-prefixed with basePath, or documenting that they must include it. - The docs are added under
docs/latest/concepts/, which is the Fresh 1.x docs surface. The Fresh 2 canary docs live underdocs/canary/the-canary-version/. Worth confirming with maintainers where this should live.
Minor nice-to-have: a test that asserts ctx.params is repopulated against the new route after a rewrite (e.g. rewrite /u/123 → /users/:id and read ctx.params.id in the target). The current tests cover route and url, but not params.
| function getRewriteUrl(currentUrl: URL, pathOrUrl: string | URL): URL { | ||
| const rewritten = pathOrUrl instanceof URL | ||
| ? new URL(pathOrUrl) | ||
| : new URL(pathOrUrl, currentUrl); |
There was a problem hiding this comment.
basePath footgun: this resolves the string target against currentUrl, which means ctx.rewrite("/new") in an app with basePath: "/base" produces a URL of /new (not /base/new) and won't match any registered routes. The basePath test on line 348 works around this by passing a full URL object.
Two reasonable options:
- Auto-prefix
this.config.basePathfor string targets that don't already start with it (mirrors how route registration works). - Document explicitly that string targets must be absolute paths including the basePath, and only
URLtargets bypass that.
Either is fine, but the current behavior is going to surprise users with a non-empty basePath.
There was a problem hiding this comment.
Let's go with the auto-prefixing to keep it consistent with the route registration
| } | ||
|
|
||
| const rewrittenUrl = getRewriteUrl(url, pathOrUrl); | ||
| const rewrittenReq = new Request(rewrittenUrl, req); |
There was a problem hiding this comment.
Two small notes on new Request(rewrittenUrl, req):
- This transfers ownership of the body stream from
reqto the rewritten request. Any middleware that planned to readctx.reqafter callingctx.rewrite()will see an already-consumed body. Worth a one-liner in the JSDoc onContext.rewrite. - For streamed request bodies, some runtimes require
{ duplex: "half" }when constructing aRequestfrom a body-bearing init. Deno's current stable runtime appears to tolerate this, but it would be safer to setduplex: "half"explicitly whenreq.body !== null.
There was a problem hiding this comment.
- True, I'll add that
- I'll change it to half when a request body is present, but I am not sure if Deno supports this fully. I assume so. The
duplexproperty is by the way not in de VScode built in types (lib.dom.d.ts) but it is in the MDN docs.
| remoteAddr: { transport: "tcp", hostname: "localhost", port: 1234 }, | ||
| }; | ||
|
|
||
| const MAX_REWRITE_COUNT = 16; |
There was a problem hiding this comment.
16 feels generous for an internal-rewrite cap — even legitimate rewrite chains rarely go past 2–3 hops. Not a blocker, but 8 would catch buggy middleware sooner with fewer wasted Request/URL allocations. Up to you.
There was a problem hiding this comment.
You're absolutely right. Lets go with 8.
| * }); | ||
| * ``` | ||
| */ | ||
| rewrite(pathOrUrl: string | URL): Promise<Response> { |
There was a problem hiding this comment.
Worth folding two facts from the prose docs into the JSDoc so they show up in editor tooltips:
- Only same-origin targets are accepted (throws otherwise).
- The rewrite transfers ownership of the request body —
ctx.reqwon't be readable afterwards in the calling middleware.
Also consider an @see cross-link to redirect() since they're conceptually paired.
|
|
||
| if (LOCALES.has(first)) { | ||
| const rewritten = `/${rest.join("/")}`; | ||
| return ctx.rewrite(rewritten === "/" ? "/" : rewritten); |
There was a problem hiding this comment.
nit: rewritten === "/" ? "/" : rewritten is a no-op — both branches return the same value. Can just be ctx.rewrite(rewritten).
There was a problem hiding this comment.
My bad, it was a hot afternoon
| }); | ||
| ``` | ||
|
|
||
| ## `.rewrite()` |
There was a problem hiding this comment.
docs/latest/ is the Fresh 1.x docs surface. Fresh 2's docs live under docs/canary/the-canary-version/, which (at the moment) doesn't have context.md / middleware.md equivalents. Worth confirming with maintainers whether this documentation should be mirrored/moved there — otherwise users on Fresh 2 won't find these docs.
There was a problem hiding this comment.
Allright, I'll create a file there and one might decide to move it to the right place.
But.. Fresh 2 isn't canary anymore right?
|
@bartlomieju Are you willing to review again? I processed your feedback! |
Currently there is no way built-in way to rewrite routes in Fresh. Other frameworks like SvelteKit and Astro do offer this feature.
There are multiple reasons why rewriting routes is a desirable feature:
I know there is currently one way to achieve rewriting without modifying Fresh' source code:
However, there are a few disadvantages to this strategy:
All suggestions are welcome! To be honest: this is my first time ever contributing to Deno and/or Fresh, and there might be better approaches. But at least this PR serves as proof it is technically possible to have rewrite middleware.