Skip to content
Merged
68 changes: 61 additions & 7 deletions packages/remix-dev/__tests__/defineRoutes-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,18 +95,72 @@ describe("defineRoutes", () => {
route("/other", "routes/other-route.tsx");
});

expect(Object.entries(routes)).toHaveLength(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this test a bit stronger by asserting that we're getting the right paths, files, and IDs on defined routes 👍

expect(routes).toMatchInlineSnapshot(`
Object {
"routes/other-route": Object {
"caseSensitive": undefined,
"file": "routes/other-route.tsx",
"id": "routes/other-route",
"index": undefined,
"parentId": undefined,
"path": "/other",
},
"user": Object {
"caseSensitive": undefined,
"file": "routes/index.tsx",
"id": "user",
"index": undefined,
"parentId": undefined,
"path": "/user",
},
"user-by-id": Object {
"caseSensitive": undefined,
"file": "routes/index.tsx",
"id": "user-by-id",
"index": undefined,
"parentId": undefined,
"path": "/user/:id",
},
}
`);
});

it("throws an error when one route already exists with the same custom ID", () => {
function defineNonUniqueRoutes() {
it("throws an error on route id collisions", () => {
// Two conflicting custom id's
let defineNonUniqueRoutes = () => {
defineRoutes((route) => {
route("/user/:id", "routes/index.tsx", { id: "user" });
route("/user", "routes/index.tsx", { id: "user" });
route("/user/:id", "routes/user.tsx", { id: "user" });
route("/user", "routes/user.tsx", { id: "user" });
route("/other", "routes/other-route.tsx");
});
}
};

expect(defineNonUniqueRoutes).toThrowErrorMatchingInlineSnapshot(
`"Unable to define routes with duplicate route id: \\"user\\""`
);

// Custom id conflicting with a later-defined auto-generated id
defineNonUniqueRoutes = () => {
defineRoutes((route) => {
route("/user/:id", "routes/user.tsx", { id: "routes/user" });
route("/user", "routes/user.tsx");
});
};

expect(defineNonUniqueRoutes).toThrowErrorMatchingInlineSnapshot(
`"Unable to define routes with duplicate route id: \\"routes/user\\""`
);

// Custom id conflicting with an earlier-defined auto-generated id
defineNonUniqueRoutes = () => {
defineRoutes((route) => {
route("/user", "routes/user.tsx");
route("/user/:id", "routes/user.tsx", { id: "routes/user" });
});
};

expect(defineNonUniqueRoutes).toThrow("must be unique");
expect(defineNonUniqueRoutes).toThrowErrorMatchingInlineSnapshot(
`"Unable to define routes with duplicate route id: \\"routes/user\\""`
);
});
});
5 changes: 2 additions & 3 deletions packages/remix-dev/config/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,9 @@ export function defineRoutes(
file,
};

if (!!options.id && options.id in routes) {
if (route.id in routes) {
throw new Error(
`You tried to define a route with custom id "${options.id}" but one ` +
"already exists. Custom route ids must be unique"
`Unable to define routes with duplicate route id: "${route.id}"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted this to account for the (very edge-case) scenario that an auto-generated route id collies with a previously-defined custom id and expanded the tests above to assert the new behavior

);
}

Expand Down