Skip to content

Commit

Permalink
fix(remix-dev): normalize /route routeId, update route collision wa…
Browse files Browse the repository at this point in the history 
…rnings (#5459)

Signed-off-by: Logan McAnsh <logan@mcan.sh>
  • Loading branch information
@mcansh
mcansh authored Feb 21, 2023
1 parent be70eea commit 99aec15
Show file tree
Hide file tree
Showing 4 changed files with 432 additions and 47 deletions.
34 changes: 34 additions & 0 deletions .changeset/flat-routes-route-fixes.md
View file
Original file line number Diff line number Diff line change
@@ -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
```
204 changes: 204 additions & 0 deletions integration/flat-routes-test.ts
View file
Original file line number Diff line number Diff line change
@@ -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: "/"`);
});
});
86 changes: 77 additions & 9 deletions packages/remix-dev/__tests__/flat-routes-test.ts
View file
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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);
});
}

Expand Down Expand Up @@ -276,7 +275,7 @@ describe("flatRoutes", () => {
[
"routes/folder/route.tsx",
{
id: "routes/folder/route",
id: "routes/folder",
parentId: "root",
path: "folder",
},
Expand Down Expand Up @@ -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 }];
});

Expand All @@ -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)
);
});
});
});
Loading

0 comments on commit 99aec15

Please sign in to comment.