Add Feature expansion .use(path, middleware) by Octo8080X · Pull Request #3044 · denoland/fresh

Octo8080X · 2025-06-16T10:49:32Z

target: #3040

Currently in progress.

iuioiua

Quick pass

src/app.ts

iuioiua · 2025-06-17T10:24:50Z

src/router.ts

 export interface Route<T> {
  path: string | URLPattern;
-  method: Method | "ALL";
+  method: Method | "ALL" | "MIDDLEWARE";


This seems odd to me. Is there an alternative?

I think there’s a way to use “ALL” instead of “MIDDLEWARE”.
Or we could allow undefined, but I feel that would have too large of an impact.

I decided to go with "ALL".

iuioiua · 2025-06-17T10:32:57Z

src/app.ts

+        const middlewareHandlers: Array<MiddlewareFn<State>> = [];
+        for (const middleware of middlewares) {
+          for (const handler of middleware.handlers) {
+            // We need to safely cast here to handle the type properly
+            middlewareHandlers.push(handler as unknown as MiddlewareFn<State>);
+          }
+        }


Suggested change

const middlewareHandlers: Array<MiddlewareFn<State>> = [];

for (const middleware of middlewares) {

for (const handler of middleware.handlers) {

// We need to safely cast here to handle the type properly

middlewareHandlers.push(handler as unknown as MiddlewareFn<State>);

}

}

const middlewareHandlers = middlewares.flatMap((middleware) =>

middleware.handlers

);

iuioiua · 2025-06-17T10:36:11Z

src/app.ts

+        // Create a new array of handlers with the correct type
+        const combinedHandlers: Array<MiddlewareFn<State>> = [
+          ...middlewareHandlers,
+          ...route.handlers.map((h) => h as unknown as MiddlewareFn<State>),
+        ];


Suggested change

// Create a new array of handlers with the correct type

const combinedHandlers: Array<MiddlewareFn<State>> = [

...middlewareHandlers,

...route.handlers.map((h) => h as unknown as MiddlewareFn<State>),

];

const combinedHandlers = [

...middlewareHandlers,

...route.handlers,

];

iuioiua · 2025-06-17T10:40:54Z

src/app.ts

+    if (
+      typeof pathOrMiddleware === "string" ||
+      pathOrMiddleware instanceof URLPattern
+    ) {
+      // First argument is a path
+      if (!middleware) {
+        throw new Error("Middleware is required when path is provided");
+      }
+      this.#router.addMiddleware(pathOrMiddleware, middleware);
+    } else {
+      this.#router.addMiddleware("/*", pathOrMiddleware);
+    }


Suggested change

if (

typeof pathOrMiddleware === "string" ||

pathOrMiddleware instanceof URLPattern

) {

// First argument is a path

if (!middleware) {

throw new Error("Middleware is required when path is provided");

}

this.#router.addMiddleware(pathOrMiddleware, middleware);

} else {

this.#router.addMiddleware("/*", pathOrMiddleware);

}

if (typeof pathOrMiddleware === "function") {

this.#router.addMiddleware("/*", pathOrMiddleware);

return this;

}

if (!middleware) {

throw new TypeError("Middleware is required when path is provided");

}

this.#router.addMiddleware(pathOrMiddleware, middleware);

Maybe a little tidier?

Thank you.
With the suggested approach, pathOrMiddleware can't be narrowed down to string | URLPattern, which causes a type error—so I decided to keep it as it is for now.

iuioiua · 2025-06-17T10:47:24Z

src/router.ts

+    if (
+      typeof pathname === "string" && pathname !== "/*" &&
+      IS_PATTERN.test(pathname)
+    ) {
+      this._middlewares.push({
+        path: new URLPattern({ pathname }),
+        handlers: [handler],
+        method: "MIDDLEWARE",
+      });
+    } else {
+      this._middlewares.push({
+        path: pathname,
+        handlers: [handler],
+        method: "MIDDLEWARE",
+      });
+    }


Suggested change

if (

typeof pathname === "string" && pathname !== "/*" &&

IS_PATTERN.test(pathname)

) {

this._middlewares.push({

path: new URLPattern({ pathname }),

handlers: [handler],

method: "MIDDLEWARE",

});

} else {

this._middlewares.push({

path: pathname,

handlers: [handler],

method: "MIDDLEWARE",

});

}

this._middlewares.push({

path: isPattern(pathname) ? new URLPattern({ pathname }) : pathname,

handlers: [handler],

method: "MIDDLEWARE",

});

We can factor out that conditional into an isPattern() which can be reused on lines 60-61.

Thanks!
I extracted it as isURLPattern.

iuioiua · 2025-06-21T05:32:28Z

src/router.ts

+      path: (
+          typeof pathname === "string" && pathname !== "/*" &&
+          !this.isURLPattern(pathname)
+        )


Nit: To clarify, my idea was to have everything inside this condition be factored out. Not just the URLPattern bit. Also, I don't see a reason why it should be a method when it doesn't use any property from this class.

Apologies — I didn’t fully grasp your intent at first.
I’ve now moved the logic out of the class and reorganized the content.

…0X/fresh into feature/add_middleware_with_path

iuioiua

LGTM! Just a few non-blocking nits.

iuioiua · 2025-06-27T00:03:31Z

src/app.ts

+      this.#router.addMiddleware("/*", pathOrMiddleware);
+    } else {
+      if (!middleware) {
+        throw new Error("Middleware is required when path is provided");


Suggested change

throw new Error("Middleware is required when path is provided");

throw new TypeError("Middleware is required when path is provided");

Nit

iuioiua · 2025-06-27T00:07:04Z

src/app.ts

-        : middlewares.concat(route.handlers);
-      this.#router.add(route.method, merged, combined);
+        // Create a new array of handlers with the correct type
+        const combinedHandlers: Array<MiddlewareFn<State>> = [


Suggested change

const combinedHandlers: Array<MiddlewareFn<State>> = [

const combinedHandlers = [

Nit: this might not be needed

iuioiua · 2025-06-27T00:13:08Z

src/app_test.tsx

Nice tests!

iuioiua · 2025-06-27T00:16:22Z

src/router.ts

+    method: Method | "ALL",
+    pathname: string | URLPattern,
+    handlers: T[],
+  ): void {


Suggested change

): void {

) {

Nit: the Deno team consider defining void and Promise<void> as explicit return types as needless boilerplate.

marvinhagemeister · 2025-07-18T16:52:04Z

Thanks for working on a PR. We've refactored the internals of Fresh quite a bite to have sort of an internal route tree when building up the middlewares. This was needed to get other features in that we needed, but does unfortunately mean that the functionality proposed in this PR has already been implemented, see #3086

Octo8080X added 5 commits June 16, 2025 11:57

edit

e787f5b

update

9dc89c7

wip

dded435

wip

f6e30ce

wip

4368e92

Octo8080X changed the title ~~Feature/add middleware with path~~ Add Feature expansion .use(path, middleware) Jun 16, 2025

iuioiua suggested changes Jun 17, 2025

View reviewed changes

Octo8080X added 6 commits June 20, 2025 00:05

update

6225d6c

update

f470727

update

fb534fa

update

8a25677

update

0d038b2

update

472fdde

iuioiua reviewed Jun 21, 2025

View reviewed changes

Octo8080X and others added 3 commits June 21, 2025 15:16

Merge branch 'main' into feature/add_middleware_with_path

9288717

update

d606c3a

Merge branch 'feature/add_middleware_with_path' of github.com:Octo808…

08332fe

…0X/fresh into feature/add_middleware_with_path

Octo8080X requested review from iuioiua and marvinhagemeister June 26, 2025 11:47

iuioiua approved these changes Jun 27, 2025

View reviewed changes

Octo8080X and others added 2 commits June 29, 2025 23:58

update nits

c120113

Merge branch 'main' into feature/add_middleware_with_path

fd94d90

marvinhagemeister closed this Jul 18, 2025

	throw new Error("Middleware is required when path is provided");
	throw new TypeError("Middleware is required when path is provided");

	const combinedHandlers: Array<MiddlewareFn<State>> = [
	const combinedHandlers = [

Conversation

Octo8080X commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iuioiua left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iuioiua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marvinhagemeister commented Jul 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Octo8080X commented Jun 16, 2025 •

edited

Loading