fix path collision detection fr pathless layout routes

remix-run · Apr 21, 2023 · 8a64e5d · 8a64e5d
1 parent 3000007
commit 8a64e5d
Show file tree

Hide file tree

Showing 3 changed files with 246 additions and 3 deletions.
diff --git a/integration/conventional-routes-test.ts b/integration/conventional-routes-test.ts
@@ -54,6 +54,23 @@ test.beforeAll(async () => {
           return <h1>Foo</h1>;
         }
       `,
+      "app/routes/nested/__pathless2.jsx": js`
+        import { Outlet } from "@remix-run/react";
+
+        export default function Layout() {
+          return (
+            <>
+              <div>Pathless 2 Layout</div>
+              <Outlet />
+            </>
+          );
+        }
+      `,
+      "app/routes/nested/__pathless2/bar.jsx": js`
+        export default function Bar() {
+          return <h1>Bar</h1>;
+        }
+      `,
     },
   });
 
@@ -102,4 +119,14 @@ function runTests() {
     expect(await app.getHtml()).toMatch("Foo");
     expect(await app.getHtml()).toMatch("Number of matches: 3");
   });
+
+  // This also asserts that we support multiple sibling pathless route layouts
+  test("displays page inside of second pathless layout", async ({ page }) => {
+    let app = new PlaywrightFixture(appFixture, page);
+    await app.goto("/nested/bar");
+    expect(await app.getHtml()).not.toMatch("Index");
+    expect(await app.getHtml()).toMatch("Pathless 2 Layout");
+    expect(await app.getHtml()).toMatch("Bar");
+    expect(await app.getHtml()).toMatch("Number of matches: 3");
+  });
 }
diff --git a/integration/route-collisions-test.ts b/integration/route-collisions-test.ts
@@ -0,0 +1,164 @@
+import { test, expect } from "@playwright/test";
+
+import { createFixture, js } from "./helpers/create-fixture";
+
+let ROOT_FILE_CONTENTS = js`
+  import { Outlet, Scripts } from "@remix-run/react";
+
+  export default function App() {
+    return (
+      <html lang="en">
+        <body>
+          <Outlet />
+          <Scripts />
+        </body>
+      </html>
+    );
+  }
+`;
+
+let LAYOUT_FILE_CONTENTS = js`
+  import { Outlet } from "@remix-run/react";
+
+  export default function Layout() {
+    return <Outlet />
+  }
+`;
+
+let LEAF_FILE_CONTENTS = js`
+  export default function Foo() {
+    return <h1>Foo</h1>;
+  }
+`;
+
+test.describe("build failures", () => {
+  let errorLogs: string[];
+  let oldConsoleError: typeof console.error;
+
+  test.beforeEach(() => {
+    errorLogs = [];
+    oldConsoleError = console.error;
+    console.error = (str) => errorLogs.push(str);
+  });
+
+  test.afterEach(() => {
+    console.error = oldConsoleError;
+  });
+
+  test("detects path collisions inside pathless layout routes", async () => {
+    try {
+      await createFixture({
+        files: {
+          "app/root.tsx": ROOT_FILE_CONTENTS,
+          "app/routes/foo.jsx": LEAF_FILE_CONTENTS,
+          "app/routes/__pathless.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/__pathless/foo.jsx": LEAF_FILE_CONTENTS,
+        },
+      });
+      expect(false).toBe(true);
+    } catch (e) {
+      expect(errorLogs[0]).toMatch(
+        'Error: Path "foo" defined by route "routes/foo" conflicts with route "routes/__pathless/foo"'
+      );
+      expect(errorLogs.length).toBe(1);
+    }
+  });
+
+  test("detects path collisions across pathless layout routes", async () => {
+    try {
+      await createFixture({
+        files: {
+          "app/root.tsx": ROOT_FILE_CONTENTS,
+          "app/routes/__pathless.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/__pathless/foo.jsx": LEAF_FILE_CONTENTS,
+          "app/routes/__pathless2.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/__pathless2/foo.jsx": LEAF_FILE_CONTENTS,
+        },
+      });
+      expect(false).toBe(true);
+    } catch (e) {
+      expect(errorLogs[0]).toMatch(
+        'Error: Path "foo" defined by route "routes/__pathless/foo" conflicts with route "routes/__pathless2/foo"'
+      );
+      expect(errorLogs.length).toBe(1);
+    }
+  });
+
+  test("detects path collisions inside multiple pathless layout routes", async () => {
+    try {
+      await createFixture({
+        files: {
+          "app/root.tsx": ROOT_FILE_CONTENTS,
+          "app/routes/foo.jsx": LEAF_FILE_CONTENTS,
+          "app/routes/__pathless.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/__pathless/__again.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/__pathless/__again/foo.jsx": LEAF_FILE_CONTENTS,
+        },
+      });
+      expect(false).toBe(true);
+    } catch (e) {
+      expect(errorLogs[0]).toMatch(
+        'Error: Path "foo" defined by route "routes/foo" conflicts with route "routes/__pathless/__again/foo"'
+      );
+      expect(errorLogs.length).toBe(1);
+    }
+  });
+
+  test("detects path collisions of index files inside pathless layouts", async () => {
+    try {
+      await createFixture({
+        files: {
+          "app/root.tsx": ROOT_FILE_CONTENTS,
+          "app/routes/index.jsx": LEAF_FILE_CONTENTS,
+          "app/routes/__pathless.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/__pathless/index.jsx": LEAF_FILE_CONTENTS,
+        },
+      });
+      expect(false).toBe(true);
+    } catch (e) {
+      expect(errorLogs[0]).toMatch(
+        'Error: Path "/" defined by route "routes/index" conflicts with route "routes/__pathless/index"'
+      );
+      expect(errorLogs.length).toBe(1);
+    }
+  });
+
+  test("detects path collisions of index files across multiple pathless layouts", async () => {
+    try {
+      await createFixture({
+        files: {
+          "app/root.tsx": ROOT_FILE_CONTENTS,
+          "app/routes/nested/__pathless.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/nested/__pathless/index.jsx": LEAF_FILE_CONTENTS,
+          "app/routes/nested/__oops.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/nested/__oops/index.jsx": LEAF_FILE_CONTENTS,
+        },
+      });
+      expect(false).toBe(true);
+    } catch (e) {
+      expect(errorLogs[0]).toMatch(
+        'Error: Path "nested" defined by route "routes/nested/__oops/index" conflicts with route "routes/nested/__pathless/index"'
+      );
+      expect(errorLogs.length).toBe(1);
+    }
+  });
+
+  test("detects path collisions of param routes inside pathless layouts", async () => {
+    try {
+      await createFixture({
+        files: {
+          "app/root.tsx": ROOT_FILE_CONTENTS,
+          "app/routes/$param.jsx": LEAF_FILE_CONTENTS,
+          "app/routes/__pathless.jsx": LAYOUT_FILE_CONTENTS,
+          "app/routes/__pathless/$param.jsx": LEAF_FILE_CONTENTS,
+        },
+      });
+      expect(false).toBe(true);
+    } catch (e) {
+      expect(errorLogs[0]).toMatch(
+        'Error: Path ":param" defined by route "routes/$param" conflicts with route "routes/__pathless/$param"'
+      );
+      expect(errorLogs.length).toBe(1);
+    }
+  });
+});
diff --git a/packages/remix-dev/config/routesConvention.ts b/packages/remix-dev/config/routesConvention.ts
@@ -73,11 +73,63 @@ export function defineConventionalRoutes(
       let isIndexRoute = routeId.endsWith("/index");
       let fullPath = createRoutePath(routeId.slice("routes".length + 1));
       let uniqueRouteId = (fullPath || "") + (isIndexRoute ? "?index" : "");
-
-      if (uniqueRouteId) {
+      let isPathlessLayoutRoute =
+        routeId.split("/").pop()?.startsWith("__") === true;
+
+      /**
+       * We do not try to detect path collisions for pathless layout route
+       * files because, by definition, they create the potential for route
+       * collisions _at that level in the tree_.  For example, consider the
+       * following route structure:
+       *
+       *   routes/
+       *     parent.tsx
+       *     parent/
+       *       __pathless.tsx
+       *
+       * The route path for both parent.tsx and parent/__pathless.tsx
+       * is the same (/parent), but it's not expected you are matching at that
+       * level.  It's up to you to handle that in your `parent.tsx` or provide
+       * an index route in your pathless route folder.
+       *
+       * Consider this more complex example where a user may want multiple
+       * pathless layout routes for different subfolders
+       *
+       *   routes/
+       *     account.tsx
+       *     account/
+       *       __public/
+       *         login.tsx
+       *         perks.tsx
+       *       __private/
+       *         orders.tsx
+       *         profile.tsx
+       *       __public.tsx
+       *       __private.tsx
+       *
+       * In order to support both a public and private layout for `/account/*`
+       * URLs, we are creating a mutually exclusive set of URLs beneath 2
+       * separate pathless layout routes.  In this case, the route paths for
+       * both account/__public.tsx and account/__private.tsx is the same
+       * (/account), but we're again not expecting to match at that level.
+       *
+       * By only ignoring this check when the final portion of the filename is
+       * pathless, we will still detect path collisions such as:
+       *
+       *   routes/parent/__pathless/foo.tsx
+       *   routes/parent/__pathless2/foo.tsx
+       *
+       * and
+       *
+       *   routes/parent/__pathless/index.tsx
+       *   routes/parent/__pathless2/index.tsx
+       */
+      if (uniqueRouteId && !isPathlessLayoutRoute) {
         if (uniqueRoutes.has(uniqueRouteId)) {
           throw new Error(
-            `Path ${JSON.stringify(fullPath)} defined by route ${JSON.stringify(
+            `Path ${JSON.stringify(
+              fullPath || "/"
+            )} defined by route ${JSON.stringify(
               routeId
             )} conflicts with route ${JSON.stringify(
               uniqueRoutes.get(uniqueRouteId)