fix: middleware regressions (#3106)

Fixes some regressions found after merging
#3086

- Missing 404 middlewares when mounting sub-apps
- revert more staticFiles logic to always require the middleware like
before

Author: marvinhagemeister

src/app.ts

@@ -30,7 +30,6 @@ import {
   segmentToMiddlewares,
 } from "./segments.ts";
 import { isHandlerByMethod, type PageResponse } from "./handlers.ts";
-import { staticFiles } from "./middlewares/static_files.ts";
 
 // TODO: Completed type clashes in older Deno versions
 // deno-lint-ignore no-explicit-any
@@ -39,9 +38,6 @@ export const DEFAULT_CONN_INFO: any = {
   remoteAddr: { transport: "tcp", hostname: "localhost", port: 1234 },
 };
 
-/** Used to group mounted apps. Only internal */
-let INTERNAL_ID = 0;
-
 const DEFAULT_RENDER = <State>(): Promise<PageResponse<State>> =>
   // deno-lint-ignore no-explicit-any
   Promise.resolve({ data: {} as any });
@@ -182,7 +178,6 @@ export class App<State> {
     method: Method | "ALL";
     pattern: string;
     fns: MiddlewareFn<State>[];
-    unshift: boolean;
   }[] = [];
 
   static {
@@ -343,38 +338,38 @@ export class App<State> {
     method: Method | "ALL",
     path: string,
     fns: MiddlewareFn<State>[],
-    unshift = false,
   ) {
     const segment = getOrCreateSegment<State>(this.#root, path, false);
     const result = segmentToMiddlewares(segment);
 
     result.push(...fns);
 
     const resPath = mergePath(this.config.basePath, path);
-    this.#addRoute(method, resPath, result, unshift);
+    this.#addRoute(method, resPath, result);
   }
 
   #addRoute(
     method: Method | "ALL",
     path: string,
     fns: MiddlewareFn<State>[],
-    unshift = false,
   ) {
-    this.#routeDefs.push({ method, pattern: path, fns, unshift });
+    this.#routeDefs.push({ method, pattern: path, fns });
   }
 
   mountApp(path: string, app: App<State>): this {
-    const segmentPath = mergePath(path, `/__${INTERNAL_ID++}/`);
+    const segmentPath = mergePath("", path);
     const segment = getOrCreateSegment(this.#root, segmentPath, true);
     const fns = segmentToMiddlewares(segment);
 
+    segment.middlewares.push(...app.#root.middlewares);
+
     const routes = app.#routeDefs;
     for (let i = 0; i < routes.length; i++) {
       const route = routes[i];
 
       const merged = mergePath(path, route.pattern);
       const mergedFns = [...fns, ...route.fns];
-      this.#addRoute(route.method, merged, mergedFns, route.unshift);
+      this.#addRoute(route.method, merged, mergedFns);
     }
 
     app.#islandRegistry.forEach((value, key) => {
@@ -408,20 +403,22 @@ export class App<State> {
       return missingBuildHandler;
     }
 
-    // Fallthrough
-    this.#addMiddleware(
-      "ALL",
-      "*",
-      [...this.#root.middlewares, staticFiles()],
-      true,
-    );
-
     for (let i = 0; i < this.#routeDefs.length; i++) {
       const route = this.#routeDefs[i];
-      this.#router.add(route.method, route.pattern, route.fns, route.unshift);
+      if (route.method === "ALL") {
+        this.#router.add("GET", route.pattern, route.fns);
+        this.#router.add("DELETE", route.pattern, route.fns);
+        this.#router.add("HEAD", route.pattern, route.fns);
+        this.#router.add("OPTIONS", route.pattern, route.fns);
+        this.#router.add("PATCH", route.pattern, route.fns);
+        this.#router.add("POST", route.pattern, route.fns);
+        this.#router.add("PUT", route.pattern, route.fns);
+      } else {
+        this.#router.add(route.method, route.pattern, route.fns);
+      }
     }
 
-    const rootMiddlewares = this.#root.middlewares;
+    const rootMiddlewares = segmentToMiddlewares(this.#root);
 
     return async (
       req: Request,

src/app_test.tsx

@@ -412,6 +412,29 @@ Deno.test(
   },
 );
 
+Deno.test("App - .mountApp() fallback route", async () => {
+  let called = "";
+  const innerApp = new App<{ text: string }>()
+    .use(function Inner(ctx) {
+      called += "_Inner";
+      return ctx.next();
+    })
+    .get("/", (ctx) => new Response(ctx.state.text));
+
+  const app = new App<{ text: string }>()
+    .use(function Outer(ctx) {
+      called += "Outer";
+      return ctx.next();
+    })
+    .mountApp("/", innerApp);
+
+  const server = new FakeServer(app.handler());
+
+  const res = await server.get("/invalid");
+  await res.body?.cancel();
+  expect(called).toEqual("Outer_Inner");
+});
+
 Deno.test("App - catches errors", async () => {
   let thrownErr: unknown | null = null;
   const app = new App<{ text: string }>()

src/router.ts

@@ -21,6 +21,18 @@ export interface DynamicRouteDef<T> {
   byMethod: RouteByMethod<T>;
 }
 
+function newByMethod<T>(): RouteByMethod<T> {
+  return {
+    GET: [],
+    POST: [],
+    PATCH: [],
+    DELETE: [],
+    PUT: [],
+    HEAD: [],
+    OPTIONS: [],
+  };
+}
+
 export interface RouteResult<T> {
   params: Record<string, string>;
   handlers: T[];
@@ -33,7 +45,6 @@ export interface Router<T> {
     method: Method | "OPTIONS" | "ALL",
     pathname: string,
     handlers: T[],
-    unshift?: boolean,
   ): void;
   match(method: Method, url: URL, init?: T[]): RouteResult<T>;
   getAllowedMethods(pattern: string): string[];
@@ -56,43 +67,25 @@ export class UrlPatternRouter<T> implements Router<T> {
   }
 
   add(
-    method: Method | "ALL",
+    method: Method,
     pathname: string,
     handlers: T[],
-    unshift = false,
   ) {
     let allowed = this.#allowed.get(pathname);
     if (allowed === undefined) {
       allowed = new Set();
       this.#allowed.set(pathname, allowed);
     }
-    if (method === "ALL") {
-      allowed.add("GET");
-      allowed.add("POST");
-      allowed.add("PATCH");
-      allowed.add("PUT");
-      allowed.add("DELETE");
-      allowed.add("HEAD");
-      allowed.add("OPTIONS");
-    } else {
-      allowed.add(method);
-    }
+
+    allowed.add(method);
 
     let byMethod: RouteByMethod<T>;
     if (IS_PATTERN.test(pathname)) {
       let def = this.#dynamics.get(pathname);
       if (def === undefined) {
         def = {
           pattern: new URLPattern({ pathname }),
-          byMethod: {
-            DELETE: [],
-            GET: [],
-            HEAD: [],
-            OPTIONS: [],
-            PATCH: [],
-            POST: [],
-            PUT: [],
-          },
+          byMethod: newByMethod(),
         };
         this.#dynamics.set(pathname, def);
         this.#dynamicArr.push(def);
@@ -104,34 +97,15 @@ export class UrlPatternRouter<T> implements Router<T> {
       if (def === undefined) {
         def = {
           pattern: pathname,
-          byMethod: {
-            DELETE: [],
-            GET: [],
-            HEAD: [],
-            OPTIONS: [],
-            PATCH: [],
-            POST: [],
-            PUT: [],
-          },
+          byMethod: newByMethod(),
         };
         this.#statics.set(pathname, def);
       }
 
       byMethod = def.byMethod;
     }
 
-    const fn = unshift ? "unshift" : "push";
-    if (method === "ALL") {
-      byMethod.DELETE[fn](...handlers);
-      byMethod.GET[fn](...handlers);
-      byMethod.HEAD[fn](...handlers);
-      byMethod.OPTIONS[fn](...handlers);
-      byMethod.PATCH[fn](...handlers);
-      byMethod.POST[fn](...handlers);
-      byMethod.PUT[fn](...handlers);
-    } else {
-      byMethod[method][fn](...handlers);
-    }
+    byMethod[method].push(...handlers);
   }
 
   match(method: Method, url: URL, init: T[] = []): RouteResult<T> {

tests/active_links_test.tsx

@@ -1,4 +1,4 @@
-import { App } from "fresh";
+import { App, staticFiles } from "fresh";
 import {
   allIslandApp,
   assertNotSelector,
@@ -27,7 +27,8 @@ function testApp<T>(): App<T> {
   const app = new App<T>()
     .island(selfCounter, "SelfCounter", SelfCounter)
     .island(partialInIsland, "PartialInIsland", PartialInIsland)
-    .island(jsonIsland, "JsonIsland", JsonIsland);
+    .island(jsonIsland, "JsonIsland", JsonIsland)
+    .use(staticFiles());
   setBuildCache(app, getBuildCache(allIslandApp));
   return app;
 }

tests/islands_test.tsx

@@ -1,4 +1,4 @@
-import { App, fsRoutes } from "fresh";
+import { App, fsRoutes, staticFiles } from "fresh";
 import { Counter } from "./fixtures_islands/Counter.tsx";
 import { IslandInIsland } from "./fixtures_islands/IslandInIsland.tsx";
 import { JsonIsland } from "./fixtures_islands/JsonIsland.tsx";
@@ -37,6 +37,7 @@ await buildProd(allIslandApp);
 function testApp(config?: FreshConfig) {
   const app = new App(config);
   setBuildCache(app, getBuildCache(allIslandApp));
+  app.use(staticFiles());
   return app;
 }
 

tests/partials_test.tsx

@@ -1,4 +1,4 @@
-import { App } from "fresh";
+import { App, staticFiles } from "fresh";
 import { Partial } from "fresh/runtime";
 import {
   allIslandApp,
@@ -40,7 +40,8 @@ function testApp<T>(): App<T> {
     .island(selfCounter, "SelfCounter", SelfCounter)
     .island(partialInIsland, "PartialInIsland", PartialInIsland)
     .island(jsonIsland, "JsonIsland", JsonIsland)
-    .island(optOutPartialLink, "OptOutPartialLink", OptOutPartialLink);
+    .island(optOutPartialLink, "OptOutPartialLink", OptOutPartialLink)
+    .use(staticFiles());
 
   setBuildCache(app, getBuildCache(allIslandApp));
   return app;

www/main.ts

@@ -1,6 +1,7 @@
-import { App, fsRoutes, trailingSlashes } from "fresh";
+import { App, fsRoutes, staticFiles, trailingSlashes } from "fresh";
 
 export const app = new App({ root: import.meta.url })
+  .use(staticFiles())
   .use(trailingSlashes("never"));
 
 await fsRoutes(app, {