fix: log warning on invalid route configurations (#2954)

Related to #2953.

I found it confusing that there were unsupported route configurations,
that fails silently without any errors or wanings. Though I think it is
a good idea to not throw for this, as that would be worse DX. But
logging a warning if the developer is migrating from Fresh v1, or if the
developer is using code that will not work (e.g. copy pasting a handler
from a route to `_middleware`), I think it's helpful where reasonable to
help guide a bit more.

Does a small refactor first to hopefully improve typing to be more
correct for `InternalRoute.handlers`, then adds warnings for the
following:

- `_middleware` using Object form (`GET`, `POST` handlers) of the
handler format
  - Helps find issues when moving handler from route to middleware
- `_layout` using `handlers`
  - E.g. if developer moves a route to a layout
- ~~`_404`, `_500` or `_error` using Object handler form (`GET`, `POST`
handlers), which is not yet supported (See #2953)~~
  - No longer necessary as it is fixed by #2955

Author: csvn

src/handlers.ts

@@ -98,9 +98,10 @@ export type RouteHandler<Data, State> =
   | HandlerByMethod<Data, State>;
 
 export function isHandlerByMethod<D, S>(
-  handler: RouteHandler<D, S>,
+  handler: RouteHandler<D, S> | RouteHandler<D, S>[] | null,
 ): handler is HandlerByMethod<D, S> {
-  return handler !== null && typeof handler === "object";
+  return handler !== null && !Array.isArray(handler) &&
+    typeof handler === "object";
 }
 
 /**

src/plugins/fs_routes/mod.ts

@@ -24,7 +24,10 @@ interface InternalRoute<State> {
   base: string;
   filePath: string;
   config: RouteConfig | null;
-  handlers: RouteHandler<unknown, State> | null;
+  handlers:
+    | RouteHandler<unknown, State>[]
+    | RouteHandler<unknown, State>
+    | null;
   component: AnyComponent<PageProps<unknown, State>> | null;
 }
 
@@ -212,10 +215,14 @@ export async function fsRoutes<State>(
     for (let k = 0; k < stack.length; k++) {
       const mod = stack[k];
       if (mod.path.endsWith("/_middleware")) {
-        if (mod.handlers !== null && !isHandlerByMethod(mod.handlers)) {
+        if (Array.isArray(mod.handlers)) {
+          middlewares.push(...(mod.handlers as MiddlewareFn<State>[]));
+        } else if (typeof mod.handlers === "function") {
           middlewares.push(mod.handlers as MiddlewareFn<State>);
-        } else if (Array.isArray(mod.handlers)) {
-          middlewares.push(...mod.handlers);
+        } else if (isHandlerByMethod(mod.handlers)) {
+          warnInvalidRoute(
+            "Middleware does not support object handlers with GET, POST, etc.",
+          );
         }
       }
 
@@ -233,6 +240,8 @@ export async function fsRoutes<State>(
       // _layouts
       if (skipLayouts && mod.path.endsWith("/_layout")) {
         continue;
+      } else if (mod.handlers !== null && mod.path.endsWith("/_layout")) {
+        warnInvalidRoute("Layout does not support handlers");
       } else if (!skipLayouts && mod.config?.skipInheritedLayouts) {
         const first = components.length > 0 ? components[0] : null;
         components = [];
@@ -296,8 +305,7 @@ export async function fsRoutes<State>(
 
     if (routeMod.component !== null) {
       components.push(routeMod.component);
-      const missingGetHandler = handlers !== null &&
-        isHandlerByMethod(handlers) &&
+      const missingGetHandler = isHandlerByMethod(handlers) &&
         !Object.keys(handlers).includes("GET");
       if (missingGetHandler) {
         const combined = middlewares.concat(
@@ -366,6 +374,14 @@ function notFoundMiddleware<State>(
   };
 }
 
+function warnInvalidRoute(message: string) {
+  // deno-lint-ignore no-console
+  console.warn(
+    `🍋 %c[WARNING] Unsupported route config: ${message}`,
+    "color:rgb(251, 184, 0)",
+  );
+}
+
 async function walkDir(
   dir: string,
   callback: (entry: WalkEntry) => void,

src/plugins/fs_routes/mod_test.tsx

@@ -8,7 +8,8 @@ import {
 } from "./mod.ts";
 import { delay, FakeServer } from "../../test_utils.ts";
 import { createFakeFs } from "../../test_utils.ts";
-import { expect } from "@std/expect";
+import { expect, fn } from "@std/expect";
+import { stub } from "@std/testing/mock";
 import { type HandlerByMethod, type HandlerFn, page } from "../../handlers.ts";
 import type { Method } from "../../router.ts";
 import { parseHtml } from "../../../tests/test_utils.tsx";
@@ -1409,3 +1410,44 @@ Deno.test("support bigint keys", async () => {
   expect(text).toContain("ok");
   expect(text).toContain("key:9007199254740991");
 });
+
+Deno.test("fsRoutes - warn on _middleware with object handler", async () => {
+  // deno-lint-ignore no-explicit-any
+  using warnSpy = stub(console, "warn", fn(() => {}) as any);
+  const server = await createServer({
+    "routes/_middleware.ts": { handler: { GET: () => new Response("ok") } },
+    "routes/index.ts": { handler: () => new Response("ok") },
+  });
+
+  await server.get("/");
+
+  expect(warnSpy.fake).toHaveBeenCalledTimes(1);
+  expect(warnSpy.fake).toHaveBeenLastCalledWith(
+    "🍋 %c[WARNING] Unsupported route config: Middleware does not support object handlers with GET, POST, etc.",
+    expect.any(String),
+  );
+});
+
+Deno.test("fsRoutes - warn on _layout handler", async () => {
+  // deno-lint-ignore no-explicit-any
+  using warnSpy = stub(console, "warn", fn(() => {}) as any);
+  const server = await createServer({
+    "routes/_layout.ts": {
+      handler: () => new Response("ok"),
+      default: (ctx) => (
+        <div>
+          <ctx.Component />
+        </div>
+      ),
+    },
+    "routes/index.ts": { default: () => <>ok</> },
+  });
+
+  await server.get("/");
+
+  expect(warnSpy.fake).toHaveBeenCalledTimes(1);
+  expect(warnSpy.fake).toHaveBeenLastCalledWith(
+    "🍋 %c[WARNING] Unsupported route config: Layout does not support handlers",
+    expect.any(String),
+  );
+});