Repository: remix-run/remix
Pull request #5459 (2023-02-15 04:07:42)

fix(remix-dev): normalize `/route` routeId, update route collision warnings
updates some missed logic so `<dir>/route` no longer conflicts in some cases.

updates conflict logic so it no longer throws errors and instead displays a nice error message about your conflicts

```log
 ⚠️ Route Path Collision: "/products/:pid"
 The following routes all define the same URL, only the first one will be used
 🟢️️ routes/products.$pid.tsx
 ⭕️️ routes/products.$productId.tsx
 ```
 ```log
 ⚠️ Route Path Collision: "/dashboard"
 The following routes all define the same URL, only the first one will be used
 🟢️️ routes/dashboard/route.tsx
 ⭕️️ routes/dashboard.tsx
 ```
 ```log
 ⚠️ Route Path Collision: "/"
 The following routes all define the same URL, only the first one will be used
 🟢️️ routes/_landing._index.tsx
 ⭕️️ routes/_dashboard._index.tsx
 ⭕️ routes/_index.tsx
 ```

<!--

👋 Hey, thanks for your interest in contributing to Remix!

If this is a simple docs change, go ahead and delete all this.

**Please ask first before starting work on any significant new features.**

It's never a fun experience to have your pull request declined after investing a
lot of time and effort into a new feature. To avoid this from happening, we
request that contributors create a
[Feature Request discussion](https://github.com/remix-run/remix/discussions/new?category=ideas)
to first discuss any significant new features.

https://github.com/remix-run/remix/blob/main/CONTRIBUTING.md

Please fill in or delete each item below:

-->

Closes: #5470
Closes: #5471 
Closes: #5495

- [ ] Docs
- [x] Tests

Testing Strategy:

- https://github.com/remix-run/remix/pull/5459/files#diff-a06b1f5f341c3dcdd91bfc12a0161adb284981209bc7ef6ea129e62a3d513f82
- https://github.com/remix-run/remix/pull/5459/files#diff-9d539a136164c63dc19ad243c45da2ae620f194929b43c0f01b039ba1fce5241

<!--
Please explain how you tested this. For example:

> This test covers this code: <link to test>

Or

> I opened up my windows machine and ran this script:
>
> ```
> npx create-remix@0.0.0-experimental-7e420ee3 --template remix my-test
> cd my-test
> npm run dev
> ```
-->


changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 756f3eb1bcb7d6f998dea6fd1b6b5452e0f81c77

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/remix-run/remix/new/logan/normalize-route?filename=.changeset/ninety-ladybugs-push.md&value=---%0A%22remix%22%3A%20patch%0A%22%40remix-run%2Fdev%22%3A%20patch%0A---%0A%0Afix(remix-dev)%3A%20normalize%20%60%2Froute%60%20routeId%0A)



Patch:
diff --git a/.changeset/flat-routes-route-fixes.md b/.changeset/flat-routes-route-fixes.md
new file mode 100644
index 000000000..d679e6c49
--- /dev/null
+++ b/.changeset/flat-routes-route-fixes.md
@@ -0,0 +1,34 @@
+---
+"remix": patch
+"@remix-run/dev": patch
+---
+
+fixes flat route inconsistencies where `route.{ext}` wasn't always being treated like `index.{ext}` when used in a folder
+
+route conflict no longer throw errors and instead display a helpful warning that we're using the first one we found.
+
+```log
+⚠️ Route Path Collision: "/products/:pid"
+
+The following routes all define the same URL, only the first one will be used
+
+🟢️️ routes/products.$pid.tsx
+⭕️️ routes/products.$productId.tsx
+```
+```log
+⚠️ Route Path Collision: "/dashboard"
+
+The following routes all define the same URL, only the first one will be used
+
+🟢️️ routes/dashboard/route.tsx
+⭕️️ routes/dashboard.tsx
+```
+```log
+⚠️ Route Path Collision: "/"
+
+The following routes all define the same URL, only the first one will be used
+
+🟢️️ routes/_landing._index.tsx
+⭕️️ routes/_dashboard._index.tsx
+⭕️ routes/_index.tsx
+```
diff --git a/packages/remix-dev/config/flat-routes.ts b/packages/remix-dev/config/flat-routes.ts
index dad8c43f7..eb9224c23 100644
--- a/packages/remix-dev/config/flat-routes.ts
+++ b/packages/remix-dev/config/flat-routes.ts
@@ -58,7 +58,6 @@ export function flatRoutesUniversal(
   prefix: string = "routes"
 ): RouteManifest {
   let routeMap = getRouteMap(appDirectory, routePaths, prefix);
-  let uniqueRoutes = new Map<string, string>();
   let routes = Array.from(routeMap.values());
 
   function defineNestedRoutes(
@@ -75,29 +74,12 @@ export function flatRoutesUniversal(
       // remove leading slash
       routePath = routePath.replace(/^\//, "");
 
-      let index = childRoute.index;
-      let fullPath = childRoute.path;
-
-      let uniqueRouteId = (fullPath || "") + (index ? "?index" : "");
-
-      if (uniqueRouteId) {
-        let conflict = uniqueRoutes.get(uniqueRouteId);
-        if (conflict) {
-          throw new Error(
-            `Path ${JSON.stringify(fullPath)} defined by route ` +
-              `${JSON.stringify(childRoute.id)} ` +
-              `conflicts with route ${JSON.stringify(conflict)}`
-          );
-        }
-        uniqueRoutes.set(uniqueRouteId, childRoute.id);
-      }
-
       let childRouteOptions: DefineRouteOptions = {
         id: path.posix.join(prefix, childRoute.id),
         index: childRoute.index ? true : undefined,
       };
 
-      if (index) {
+      if (childRoute.index) {
         let invalidChildRoutes = routes.filter(
           (routeInfo) => routeInfo.parentId === childRoute.id
         );
@@ -141,17 +123,6 @@ export function getRouteSegments(routeId: string) {
   let routeSegment = "";
   let rawRouteSegment = "";
   let state: State = "NORMAL";
-  let hasFolder = routeId.includes(path.posix.sep);
-
-  /**
-   * @see https://github.com/remix-run/remix/pull/5160#issuecomment-1402157424
-   */
-  if (hasFolder && (routeId.endsWith("/index") || routeId.endsWith("/route"))) {
-    let last = routeId.lastIndexOf(path.posix.sep);
-    if (last >= 0) {
-      routeId = routeId.substring(0, last);
-    }
-  }
 
   let pushRouteSegment = (segment: string, rawSegment: string) => {
     if (!segment) return;
@@ -289,7 +260,7 @@ function findParentRouteId(
   return undefined;
 }
 
-function getRouteInfo(
+export function getRouteInfo(
   appDirectory: string,
   routeDirectory: string,
   filePath: string
@@ -351,21 +322,91 @@ function getRouteMap(
 ): Readonly<Map<string, RouteInfo>> {
   let routeMap = new Map<string, RouteInfo>();
   let nameMap = new Map<string, RouteInfo>();
+  let uniqueRoutes = new Map<string, RouteInfo>();
+  let conflicts = new Map<string, RouteInfo[]>();
 
   for (let routePath of routePaths) {
     let routesDirectory = path.join(appDirectory, prefix);
     let pathWithoutAppRoutes = routePath.slice(routesDirectory.length + 1);
     if (isRouteModuleFile(pathWithoutAppRoutes)) {
       let routeInfo = getRouteInfo(appDirectory, prefix, routePath);
+
+      let uniqueRouteId =
+        (routeInfo.path || "") + (routeInfo.index ? "?index" : "");
+
+      if (uniqueRouteId) {
+        let conflict = uniqueRoutes.get(uniqueRouteId);
+        // collect conflicts for later reporting
+        if (conflict) {
+          let currentConflicts = conflicts.get(routeInfo.path || "/");
+          if (!currentConflicts) {
+            conflicts.set(routeInfo.path || "/", [conflict, routeInfo]);
+          } else {
+            currentConflicts.push(routeInfo);
+            conflicts.set(routeInfo.path || "/", currentConflicts);
+          }
+
+          continue;
+        }
+        uniqueRoutes.set(uniqueRouteId, routeInfo);
+      }
+
       routeMap.set(routeInfo.id, routeInfo);
       nameMap.set(routeInfo.name, routeInfo);
     }
   }
 
-  // update parentIds for all routes
-  for (let routeInfo of routeMap.values()) {
-    let parentId = findParentRouteId(routeInfo, nameMap);
-    routeInfo.parentId = parentId;
+  let routes = Array.from(routeMap.values()).sort((a, b) => {
+    return b.segments.length - a.segments.length;
+  });
+
+  for (let i = 0; i < routes.length; i++) {
+    let routeInfo = routes[i];
+    // update parentIds for all routes
+    routeInfo.parentId = findParentRouteId(routeInfo, nameMap);
+
+    // remove routes that conflict with other routes
+    let nextRouteInfo = routes[i + 1];
+    if (!nextRouteInfo) continue;
+
+    let segments = routeInfo.segments;
+    let nextSegments = nextRouteInfo.segments;
+    // if segment count is different, there can't be a conflict
+    if (segments.length !== nextSegments.length) continue;
+
+    for (let k = 0; k < segments.length; k++) {
+      let segment = segments[k];
+      let nextSegment = nextSegments[k];
+
+      // if segments are different, but they're both dynamic, there's a conflict
+      if (
+        segment !== nextSegment &&
+        segment.startsWith(":") &&
+        nextSegment.startsWith(":")
+      ) {
+        let currentConflicts = conflicts.get(routeInfo.path || "/");
+        // collect conflicts for later reporting, marking the next route as the conflicting route
+        if (!currentConflicts) {
+          conflicts.set(routeInfo.path || "/", [routeInfo, nextRouteInfo]);
+        } else {
+          currentConflicts.push(nextRouteInfo);
+          conflicts.set(routeInfo.path || "/", currentConflicts);
+        }
+
+        // remove conflicting route
+        routeMap.delete(nextRouteInfo.id);
+
+        continue;
+      }
+    }
+  }
+
+  // report conflicts
+  if (conflicts.size > 0) {
+    for (let [path, routes] of conflicts.entries()) {
+      let filePaths = routes.map((r) => r.file);
+      console.error(getRouteConflictErrorMessage(path, filePaths));
+    }
   }
 
   return routeMap;
@@ -381,10 +422,48 @@ function isRouteModuleFile(filePath: string) {
   return basename.endsWith(`/route`) || basename.endsWith(`/index`);
 }
 
-function createFlatRouteId(filePath: string) {
+/**
+ * @see https://github.com/remix-run/remix/pull/5160#issuecomment-1402157424
+ * normalize routeId
+ * remove `/index` and `/route` suffixes
+ * they should be treated like if they weren't folders
+ * e.g. `/dashboard` and `/dashboard/index` should be the same route
+ * e.g. `/dashboard` and `/dashboard/route` should be the same route
+ */
+function isRouteInFolder(routeId: string) {
+  return (
+    (routeId.endsWith("/index") || routeId.endsWith("/route")) &&
+    routeId.includes(path.posix.sep)
+  );
+}
+
+export function createFlatRouteId(filePath: string) {
   let routeId = createRouteId(filePath);
-  if (routeId.includes(path.posix.sep) && routeId.endsWith("/index")) {
-    routeId = routeId.split(path.posix.sep).slice(0, -1).join(path.posix.sep);
+
+  if (isRouteInFolder(routeId)) {
+    let last = routeId.lastIndexOf(path.posix.sep);
+    if (last >= 0) {
+      routeId = routeId.substring(0, last);
+    }
   }
   return routeId;
 }
+
+export function normalizePath(filePath: string) {
+  return filePath.split("/").join(path.sep);
+}
+
+export function getRouteConflictErrorMessage(
+  pathname: string,
+  routes: string[]
+) {
+  let [taken, ...others] = routes;
+
+  return (
+    `⚠️ Route Path Collision: "${pathname}"\n\n` +
+    `The following routes all define the same URL, only the first one will be used\n\n` +
+    `🟢 ${normalizePath(taken)}\n` +
+    others.map((route) => `⭕️️ ${normalizePath(route)}`).join("\n") +
+    "\n"
+  );
+}
diff --git a/packages/remix-server-runtime/routes.ts b/packages/remix-server-runtime/routes.ts
index c90802e1c..6ab00f702 100644
--- a/packages/remix-server-runtime/routes.ts
+++ b/packages/remix-server-runtime/routes.ts
@@ -4,7 +4,6 @@ import type {
   LoaderFunctionArgs,
 } from "@remix-run/router";
 
-import type { AppLoadContext } from "./data";
 import { callRouteActionRR, callRouteLoaderRR } from "./data";
 import type { FutureConfig } from "./entry";
 import type { ServerRouteModule } from "./routeModules";


Test patch:
diff --git a/integration/flat-routes-test.ts b/integration/flat-routes-test.ts
new file mode 100644
index 000000000..9254ca16e
--- /dev/null
+++ b/integration/flat-routes-test.ts
@@ -0,0 +1,204 @@
+import { PassThrough } from "node:stream";
+import { test, expect } from "@playwright/test";
+
+import { PlaywrightFixture } from "./helpers/playwright-fixture";
+import type { Fixture, AppFixture } from "./helpers/create-fixture";
+import { createFixtureProject } from "./helpers/create-fixture";
+import { createAppFixture, createFixture, js } from "./helpers/create-fixture";
+
+let fixture: Fixture;
+let appFixture: AppFixture;
+
+test.describe("flat routes", () => {
+  test.beforeAll(async () => {
+    fixture = await createFixture({
+      future: { v2_routeConvention: true },
+      files: {
+        "app/root.jsx": js`
+          import { Links, Meta, Outlet, Scripts } from "@remix-run/react";
+
+          export default function Root() {
+            return (
+              <html lang="en">
+                <head>
+                  <Meta />
+                  <Links />
+                </head>
+                <body>
+                  <div id="content">
+                    <h1>Root</h1>
+                    <Outlet />
+                  </div>
+                  <Scripts />
+                </body>
+              </html>
+            );
+          }
+        `,
+
+        "app/routes/_index.jsx": js`
+          export default function () {
+            return <h2>Index</h2>;
+          }
+        `,
+
+        "app/routes/folder/route.jsx": js`
+          export default function () {
+            return <h2>Folder (Route.jsx)</h2>;
+          }
+        `,
+
+        "app/routes/folder2/index.jsx": js`
+          export default function () {
+            return <h2>Folder (Index.jsx)</h2>;
+          }
+        `,
+
+        "app/routes/flat.file.jsx": js`
+          export default function () {
+            return <h2>Flat File</h2>;
+          }
+        `,
+
+        "app/routes/dashboard/route.jsx": js`
+          import { Outlet } from "@remix-run/react";
+
+          export default function () {
+            return (
+              <>
+                <h2>Dashboard Layout</h2>
+                <Outlet />
+              </>
+            )
+          }
+        `,
+
+        "app/routes/dashboard._index/route.jsx": js`
+          export default function () {
+            return <h3>Dashboard Index</h3>;
+          }
+        `,
+      },
+    });
+
+    appFixture = await createAppFixture(fixture);
+  });
+
+  test.afterAll(() => {
+    appFixture.close();
+  });
+
+  test.describe("without JavaScript", () => {
+    test.use({ javaScriptEnabled: false });
+    runTests();
+  });
+
+  test.describe("with JavaScript", () => {
+    test.use({ javaScriptEnabled: true });
+    runTests();
+  });
+
+  function runTests() {
+    test("renders matching routes (index)", async ({ page }) => {
+      let app = new PlaywrightFixture(appFixture, page);
+      await app.goto("/");
+      expect(await app.getHtml("#content")).toBe(`<div id="content">
+  <h1>Root</h1>
+  <h2>Index</h2>
+</div>`);
+    });
+
+    test("renders matching routes (folder route.jsx)", async ({ page }) => {
+      let app = new PlaywrightFixture(appFixture, page);
+      await app.goto("/folder");
+      expect(await app.getHtml("#content")).toBe(`<div id="content">
+  <h1>Root</h1>
+  <h2>Folder (Route.jsx)</h2>
+</div>`);
+    });
+
+    test("renders matching routes (folder index.jsx)", async ({ page }) => {
+      let app = new PlaywrightFixture(appFixture, page);
+      await app.goto("/folder2");
+      expect(await app.getHtml("#content")).toBe(`<div id="content">
+  <h1>Root</h1>
+  <h2>Folder (Index.jsx)</h2>
+</div>`);
+    });
+
+    test("renders matching routes (flat file)", async ({ page }) => {
+      let app = new PlaywrightFixture(appFixture, page);
+      await app.goto("/flat/file");
+      expect(await app.getHtml("#content")).toBe(`<div id="content">
+  <h1>Root</h1>
+  <h2>Flat File</h2>
+</div>`);
+    });
+
+    test("renders matching routes (nested)", async ({ page }) => {
+      let app = new PlaywrightFixture(appFixture, page);
+      await app.goto("/dashboard");
+      expect(await app.getHtml("#content")).toBe(`<div id="content">
+  <h1>Root</h1>
+  <h2>Dashboard Layout</h2>
+  <h3>Dashboard Index</h3>
+</div>`);
+    });
+  }
+});
+
+test.describe("emits warnings for route conflicts", async () => {
+  let buildStdio = new PassThrough();
+  let buildOutput: string;
+
+  let originalConsoleLog = console.log;
+  let originalConsoleWarn = console.warn;
+  let originalConsoleError = console.error;
+
+  test.beforeAll(async () => {
+    console.log = () => {};
+    console.warn = () => {};
+    console.error = () => {};
+    await createFixtureProject({
+      buildStdio,
+      future: { v2_routeConvention: true },
+      files: {
+        "routes/_dashboard._index.tsx": js`
+          export default function () {
+            return <p>routes/_dashboard._index</p>;
+          }
+        `,
+        "app/routes/_index.jsx": js`
+          export default function () {
+            return <p>routes._index</p>;
+          }
+        `,
+        "app/routes/_landing._index.jsx": js`
+          export default function () {
+            return <p>routes/_landing._index</p>;
+          }
+        `,
+      },
+    });
+
+    let chunks: Buffer[] = [];
+    buildOutput = await new Promise<string>((resolve, reject) => {
+      buildStdio.on("data", (chunk) => chunks.push(Buffer.from(chunk)));
+      buildStdio.on("error", (err) => reject(err));
+      buildStdio.on("end", () =>
+        resolve(Buffer.concat(chunks).toString("utf8"))
+      );
+    });
+  });
+
+  test.afterAll(() => {
+    console.log = originalConsoleLog;
+    console.warn = originalConsoleWarn;
+    console.error = originalConsoleError;
+  });
+
+  test("warns about conflicting routes", () => {
+    console.log(buildOutput);
+    expect(buildOutput).toContain(`⚠️ Route Path Collision: "/"`);
+  });
+});
diff --git a/packages/remix-dev/__tests__/flat-routes-test.ts b/packages/remix-dev/__tests__/flat-routes-test.ts
index 5c55ab790..437d1e321 100644
--- a/packages/remix-dev/__tests__/flat-routes-test.ts
+++ b/packages/remix-dev/__tests__/flat-routes-test.ts
@@ -1,10 +1,11 @@
 import path from "node:path";
 
 import {
-  createRoutePath,
   flatRoutesUniversal,
+  getRouteConflictErrorMessage,
+  getRouteInfo,
   getRouteSegments,
-  isIndexRoute,
+  normalizePath,
 } from "../config/flat-routes";
 import type { ConfigRoute } from "../config/routes";
 
@@ -88,11 +89,9 @@ describe("flatRoutes", () => {
 
     for (let [input, expected] of tests) {
       it(`"${input}" -> "${expected}"`, () => {
-        let isIndex = isIndexRoute(input);
-        let [routeSegments, rawRouteSegments] = getRouteSegments(input);
-        expect(createRoutePath(routeSegments, rawRouteSegments, isIndex)).toBe(
-          expected
-        );
+        let fullRoutePath = path.join(APP_DIR, "routes", `${input}.tsx`);
+        let routeInfo = getRouteInfo(APP_DIR, "routes", fullRoutePath);
+        expect(routeInfo.path).toBe(expected);
       });
     }
 
@@ -276,7 +275,7 @@ describe("flatRoutes", () => {
       [
         "routes/folder/route.tsx",
         {
-          id: "routes/folder/route",
+          id: "routes/folder",
           parentId: "root",
           path: "folder",
         },
@@ -601,7 +600,7 @@ describe("flatRoutes", () => {
     ];
 
     let files: [string, ConfigRoute][] = testFiles.map(([file, route]) => {
-      let filepath = file.split("/").join(path.sep);
+      let filepath = normalizePath(file);
       return [filepath, { ...route, file: filepath }];
     });
 
@@ -619,4 +618,73 @@ describe("flatRoutes", () => {
       });
     }
   });
+
+  describe("warns when there's a route collision", () => {
+    let consoleError = jest
+      .spyOn(global.console, "error")
+      .mockImplementation(() => {});
+
+    afterEach(consoleError.mockReset);
+
+    test("index files", () => {
+      // we'll add file manually before running the tests
+      let testFiles = [
+        "routes/_dashboard._index.tsx",
+        "routes/_landing._index.tsx",
+        "routes/_index.tsx",
+      ];
+
+      let routeManifest = flatRoutesUniversal(
+        APP_DIR,
+        testFiles.map((file) => path.join(APP_DIR, normalizePath(file)))
+      );
+
+      let routes = Object.values(routeManifest);
+
+      // we had a collision as /route and /index are the same
+      expect(routes).toHaveLength(1);
+      expect(consoleError).toHaveBeenCalledWith(
+        getRouteConflictErrorMessage("/", testFiles)
+      );
+    });
+
+    test("folder/route.tsx matching folder.tsx", () => {
+      // we'll add file manually before running the tests
+      let testFiles = ["routes/dashboard.tsx", "routes/dashboard/route.tsx"];
+
+      let routeManifest = flatRoutesUniversal(
+        APP_DIR,
+        testFiles.map((file) => path.join(APP_DIR, normalizePath(file)))
+      );
+
+      let routes = Object.values(routeManifest);
+
+      // we had a collision as /route and /index are the same
+      expect(routes).toHaveLength(1);
+      expect(consoleError).toHaveBeenCalledWith(
+        getRouteConflictErrorMessage("/dashboard", testFiles)
+      );
+    });
+
+    test("same path, different param name", () => {
+      // we'll add file manually before running the tests
+      let testFiles = [
+        "routes/products.$pid.tsx",
+        "routes/products.$productId.tsx",
+      ];
+
+      let routeManifest = flatRoutesUniversal(
+        APP_DIR,
+        testFiles.map((file) => path.join(APP_DIR, normalizePath(file)))
+      );
+
+      let routes = Object.values(routeManifest);
+
+      // we had a collision as /route and /index are the same
+      expect(routes).toHaveLength(1);
+      expect(consoleError).toHaveBeenCalledWith(
+        getRouteConflictErrorMessage("/products/:pid", testFiles)
+      );
+    });
+  });
 });