From f80267df6238c0dff84528f71363d76213914cd4 Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Tue, 9 Aug 2022 18:13:17 -0300 Subject: [PATCH 01/10] Allow multiple routes for same route module --- contributors.yml | 1 + .../remix-dev/__tests__/defineRoutes-test.ts | 10 ++++ packages/remix-dev/compiler/assets.ts | 49 ++++++++++++------- packages/remix-dev/config/routes.ts | 11 ++++- 4 files changed, 52 insertions(+), 19 deletions(-) diff --git a/contributors.yml b/contributors.yml index 205886e7c6f..52b1e8468bf 100644 --- a/contributors.yml +++ b/contributors.yml @@ -236,6 +236,7 @@ - lpsinger - lswest - lucasdibz +- lucasferreira - luispagarcia - luistak - luistorres diff --git a/packages/remix-dev/__tests__/defineRoutes-test.ts b/packages/remix-dev/__tests__/defineRoutes-test.ts index d4766d004f5..cac3f58805d 100644 --- a/packages/remix-dev/__tests__/defineRoutes-test.ts +++ b/packages/remix-dev/__tests__/defineRoutes-test.ts @@ -87,4 +87,14 @@ describe("defineRoutes", () => { } `); }); + + it("allows multiple routes with the same route module", () => { + let routes = defineRoutes((route) => { + route("/user/:id", "routes/index.tsx", { id: "user-by-id" }); + route("/user", "routes/index.tsx", { id: "user" }); + route("/other", "routes/other-route.tsx"); + }); + + expect(Object.entries(routes)).toHaveLength(3); + }); }); diff --git a/packages/remix-dev/compiler/assets.ts b/packages/remix-dev/compiler/assets.ts index 678a2f93f7d..7378696eca5 100644 --- a/packages/remix-dev/compiler/assets.ts +++ b/packages/remix-dev/compiler/assets.ts @@ -56,10 +56,13 @@ export async function createAssetsManifest( config.appDirectory, config.entryClientFile ); - let routesByFile: Map = Object.keys(config.routes).reduce( + let routesByFile: Map = Object.keys(config.routes).reduce( (map, key) => { let route = config.routes[key]; - map.set(route.file, route); + map.set( + route.file, + map.has(route.file) ? [...map.get(route.file), route] : [route] + ); return map; }, new Map() @@ -85,22 +88,32 @@ export async function createAssetsManifest( /(^browser-route-module:|\?browser$)/g, "" ); - let route = routesByFile.get(entryPointFile); - invariant(route, `Cannot get route for entry point ${output.entryPoint}`); - let sourceExports = await getRouteModuleExportsCached(config, route.id); - routes[route.id] = { - id: route.id, - parentId: route.parentId, - path: route.path, - index: route.index, - caseSensitive: route.caseSensitive, - module: resolveUrl(key), - imports: resolveImports(output.imports), - hasAction: sourceExports.includes("action"), - hasLoader: sourceExports.includes("loader"), - hasCatchBoundary: sourceExports.includes("CatchBoundary"), - hasErrorBoundary: sourceExports.includes("ErrorBoundary"), - }; + let groupedRoute = routesByFile.get(entryPointFile); + invariant( + groupedRoute, + `Cannot get route(s) for entry point ${output.entryPoint}` + ); + if (groupedRoute) { + for (let route of groupedRoute) { + let sourceExports = await getRouteModuleExportsCached( + config, + route.id + ); + routes[route.id] = { + id: route.id, + parentId: route.parentId, + path: route.path, + index: route.index, + caseSensitive: route.caseSensitive, + module: resolveUrl(key), + imports: resolveImports(output.imports), + hasAction: sourceExports.includes("action"), + hasLoader: sourceExports.includes("loader"), + hasCatchBoundary: sourceExports.includes("CatchBoundary"), + hasErrorBoundary: sourceExports.includes("ErrorBoundary"), + }; + } + } } } diff --git a/packages/remix-dev/config/routes.ts b/packages/remix-dev/config/routes.ts index 9d97158d03f..b06aa841adb 100644 --- a/packages/remix-dev/config/routes.ts +++ b/packages/remix-dev/config/routes.ts @@ -54,6 +54,13 @@ export interface DefineRouteOptions { * Should be `true` if this is an index route that does not allow child routes. */ index?: boolean; + + /** + * Should be a optional unique id string for this route, named like its `file` + * but without the extension. Use this optional param if you need to aggregate two + * or more routes to the same route file. + */ + id?: string; } interface DefineRouteChildren { @@ -137,11 +144,13 @@ export function defineRoutes( options = optionsOrChildren || {}; } + let id = createRouteId(file); + let route: ConfigRoute = { path: path ? path : undefined, index: options.index ? true : undefined, caseSensitive: options.caseSensitive ? true : undefined, - id: createRouteId(file), + id: options.id ? `${options.id}--${id}` : id, parentId: parentRoutes.length > 0 ? parentRoutes[parentRoutes.length - 1].id From 3e93d21b5a31f146005ccf3712222fe71b2aecc8 Mon Sep 17 00:00:00 2001 From: Lucas Ferreira Date: Wed, 10 Aug 2022 09:56:58 -0300 Subject: [PATCH 02/10] Update packages/remix-dev/config/routes.ts Co-authored-by: Andrew Leedham --- packages/remix-dev/config/routes.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/remix-dev/config/routes.ts b/packages/remix-dev/config/routes.ts index b06aa841adb..b2ac7a6f0dc 100644 --- a/packages/remix-dev/config/routes.ts +++ b/packages/remix-dev/config/routes.ts @@ -56,9 +56,9 @@ export interface DefineRouteOptions { index?: boolean; /** - * Should be a optional unique id string for this route, named like its `file` - * but without the extension. Use this optional param if you need to aggregate two - * or more routes to the same route file. + * An optional unique id string for this route, appended to the file name e.g: + * `--`. Use this if you need to aggregate two or more routes + * with the same route file. */ id?: string; } From 2b86094faec3a57482251ecb98fe5279941aeb19 Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Wed, 10 Aug 2022 10:29:01 -0300 Subject: [PATCH 03/10] Update routes.ts - Better name for automated ID variable; - Small adjust in `id` option comment; --- packages/remix-dev/config/routes.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/remix-dev/config/routes.ts b/packages/remix-dev/config/routes.ts index b2ac7a6f0dc..0057814b792 100644 --- a/packages/remix-dev/config/routes.ts +++ b/packages/remix-dev/config/routes.ts @@ -56,9 +56,9 @@ export interface DefineRouteOptions { index?: boolean; /** - * An optional unique id string for this route, appended to the file name e.g: - * `--`. Use this if you need to aggregate two or more routes - * with the same route file. + * An optional unique id string for this route, that will be appended to the file + * name e.g: `--`. Use this if you need to aggregate two or more + * routes with the same route file. */ id?: string; } @@ -144,13 +144,13 @@ export function defineRoutes( options = optionsOrChildren || {}; } - let id = createRouteId(file); + let fileId = createRouteId(file); let route: ConfigRoute = { path: path ? path : undefined, index: options.index ? true : undefined, caseSensitive: options.caseSensitive ? true : undefined, - id: options.id ? `${options.id}--${id}` : id, + id: options.id ? `${fileId}--${options.id}` : fileId, parentId: parentRoutes.length > 0 ? parentRoutes[parentRoutes.length - 1].id From ed5148e40cef5db4e14a80a0b725a153aeb7a531 Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Thu, 22 Sep 2022 12:02:00 -0300 Subject: [PATCH 04/10] - Removing redundant IF --- packages/remix-dev/compiler/assets.ts | 35 ++++++++++++--------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/packages/remix-dev/compiler/assets.ts b/packages/remix-dev/compiler/assets.ts index 7378696eca5..3f9490ca467 100644 --- a/packages/remix-dev/compiler/assets.ts +++ b/packages/remix-dev/compiler/assets.ts @@ -93,26 +93,21 @@ export async function createAssetsManifest( groupedRoute, `Cannot get route(s) for entry point ${output.entryPoint}` ); - if (groupedRoute) { - for (let route of groupedRoute) { - let sourceExports = await getRouteModuleExportsCached( - config, - route.id - ); - routes[route.id] = { - id: route.id, - parentId: route.parentId, - path: route.path, - index: route.index, - caseSensitive: route.caseSensitive, - module: resolveUrl(key), - imports: resolveImports(output.imports), - hasAction: sourceExports.includes("action"), - hasLoader: sourceExports.includes("loader"), - hasCatchBoundary: sourceExports.includes("CatchBoundary"), - hasErrorBoundary: sourceExports.includes("ErrorBoundary"), - }; - } + for (let route of groupedRoute) { + let sourceExports = await getRouteModuleExportsCached(config, route.id); + routes[route.id] = { + id: route.id, + parentId: route.parentId, + path: route.path, + index: route.index, + caseSensitive: route.caseSensitive, + module: resolveUrl(key), + imports: resolveImports(output.imports), + hasAction: sourceExports.includes("action"), + hasLoader: sourceExports.includes("loader"), + hasCatchBoundary: sourceExports.includes("CatchBoundary"), + hasErrorBoundary: sourceExports.includes("ErrorBoundary"), + }; } } } From 9228db09268d3b4f1ede5e4bc9bcca82770ce38c Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Mon, 31 Oct 2022 17:56:13 -0300 Subject: [PATCH 05/10] Update routes.ts Revert complex custom ID in routes --- packages/remix-dev/config/routes.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/remix-dev/config/routes.ts b/packages/remix-dev/config/routes.ts index 0057814b792..57fc9812c20 100644 --- a/packages/remix-dev/config/routes.ts +++ b/packages/remix-dev/config/routes.ts @@ -56,9 +56,8 @@ export interface DefineRouteOptions { index?: boolean; /** - * An optional unique id string for this route, that will be appended to the file - * name e.g: `--`. Use this if you need to aggregate two or more - * routes with the same route file. + * An optional unique id string for this route. Use this if you need to aggregate + * two or more routes with the same route file. */ id?: string; } @@ -144,13 +143,11 @@ export function defineRoutes( options = optionsOrChildren || {}; } - let fileId = createRouteId(file); - let route: ConfigRoute = { path: path ? path : undefined, index: options.index ? true : undefined, caseSensitive: options.caseSensitive ? true : undefined, - id: options.id ? `${fileId}--${options.id}` : fileId, + id: options.id || createRouteId(file), parentId: parentRoutes.length > 0 ? parentRoutes[parentRoutes.length - 1].id From 37386629a4e62c7bc6579c14e386696c90048b80 Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Mon, 31 Oct 2022 18:28:35 -0300 Subject: [PATCH 06/10] Non unique custom routes ID error and test --- packages/remix-dev/__tests__/defineRoutes-test.ts | 12 ++++++++++++ packages/remix-dev/config/routes.ts | 7 +++++++ 2 files changed, 19 insertions(+) diff --git a/packages/remix-dev/__tests__/defineRoutes-test.ts b/packages/remix-dev/__tests__/defineRoutes-test.ts index cac3f58805d..45c7c7f2178 100644 --- a/packages/remix-dev/__tests__/defineRoutes-test.ts +++ b/packages/remix-dev/__tests__/defineRoutes-test.ts @@ -97,4 +97,16 @@ describe("defineRoutes", () => { expect(Object.entries(routes)).toHaveLength(3); }); + + it("throws an error when one route already exists with the same custom ID", () => { + function defineNonUniqueRoutes() { + defineRoutes((route) => { + route("/user/:id", "routes/index.tsx", { id: "user" }); + route("/user", "routes/index.tsx", { id: "user" }); + route("/other", "routes/other-route.tsx"); + }); + } + + expect(defineNonUniqueRoutes).toThrow("must be unique"); + }); }); diff --git a/packages/remix-dev/config/routes.ts b/packages/remix-dev/config/routes.ts index 57fc9812c20..ad97d4f58ca 100644 --- a/packages/remix-dev/config/routes.ts +++ b/packages/remix-dev/config/routes.ts @@ -155,6 +155,13 @@ export function defineRoutes( file, }; + if (!!options.id && options.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" + ); + } + routes[route.id] = route; if (children) { From 2064c57032e56c8709053c8f12c304aae932ef5d Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Mon, 31 Oct 2022 18:40:07 -0300 Subject: [PATCH 07/10] Update assets.ts Trying to solve a conflict --- packages/remix-dev/compiler/assets.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/remix-dev/compiler/assets.ts b/packages/remix-dev/compiler/assets.ts index 3f9490ca467..f90155b4e6a 100644 --- a/packages/remix-dev/compiler/assets.ts +++ b/packages/remix-dev/compiler/assets.ts @@ -3,7 +3,7 @@ import type * as esbuild from "esbuild"; import type { RemixConfig } from "../config"; import invariant from "../invariant"; -import { getRouteModuleExportsCached } from "./routes"; +import { getRouteModuleExports } from "./routeExports"; import { getHash } from "./utils/crypto"; import { createUrl } from "./utils/url"; @@ -94,7 +94,7 @@ export async function createAssetsManifest( `Cannot get route(s) for entry point ${output.entryPoint}` ); for (let route of groupedRoute) { - let sourceExports = await getRouteModuleExportsCached(config, route.id); + let sourceExports = await getRouteModuleExports(config, route.id); routes[route.id] = { id: route.id, parentId: route.parentId, From 9318a4e5606447bb3ed578b675740751d0d24f0c Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Mon, 31 Oct 2022 18:40:48 -0300 Subject: [PATCH 08/10] Revert "Update assets.ts" This reverts commit 2064c57032e56c8709053c8f12c304aae932ef5d. --- packages/remix-dev/compiler/assets.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/remix-dev/compiler/assets.ts b/packages/remix-dev/compiler/assets.ts index f90155b4e6a..3f9490ca467 100644 --- a/packages/remix-dev/compiler/assets.ts +++ b/packages/remix-dev/compiler/assets.ts @@ -3,7 +3,7 @@ import type * as esbuild from "esbuild"; import type { RemixConfig } from "../config"; import invariant from "../invariant"; -import { getRouteModuleExports } from "./routeExports"; +import { getRouteModuleExportsCached } from "./routes"; import { getHash } from "./utils/crypto"; import { createUrl } from "./utils/url"; @@ -94,7 +94,7 @@ export async function createAssetsManifest( `Cannot get route(s) for entry point ${output.entryPoint}` ); for (let route of groupedRoute) { - let sourceExports = await getRouteModuleExports(config, route.id); + let sourceExports = await getRouteModuleExportsCached(config, route.id); routes[route.id] = { id: route.id, parentId: route.parentId, From 1b554a244d0e327f176223ce229cf89a2d56d770 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 10 Nov 2022 10:52:23 -0500 Subject: [PATCH 09/10] Error on collisions with non-custom routeIds --- .../remix-dev/__tests__/defineRoutes-test.ts | 68 +++++++++++++++++-- packages/remix-dev/config/routes.ts | 5 +- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/packages/remix-dev/__tests__/defineRoutes-test.ts b/packages/remix-dev/__tests__/defineRoutes-test.ts index 45c7c7f2178..dbd73350dfa 100644 --- a/packages/remix-dev/__tests__/defineRoutes-test.ts +++ b/packages/remix-dev/__tests__/defineRoutes-test.ts @@ -95,18 +95,72 @@ describe("defineRoutes", () => { route("/other", "routes/other-route.tsx"); }); - expect(Object.entries(routes)).toHaveLength(3); + 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\\""` + ); }); }); diff --git a/packages/remix-dev/config/routes.ts b/packages/remix-dev/config/routes.ts index ad97d4f58ca..7f1e5696ef3 100644 --- a/packages/remix-dev/config/routes.ts +++ b/packages/remix-dev/config/routes.ts @@ -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}"` ); } From 86b8720ae18724eb103dccf87f9df7d472b3a2da Mon Sep 17 00:00:00 2001 From: lucasferreira Date: Wed, 30 Nov 2022 23:38:52 -0300 Subject: [PATCH 10/10] Create big-spoons-grab.md --- .changeset/big-spoons-grab.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/big-spoons-grab.md diff --git a/.changeset/big-spoons-grab.md b/.changeset/big-spoons-grab.md new file mode 100644 index 00000000000..ab0ddcd25df --- /dev/null +++ b/.changeset/big-spoons-grab.md @@ -0,0 +1,5 @@ +--- +"@remix-run/dev": minor +--- + +allow defining multiple routes for the same route module file