sortRoutes shouldn't have a default baseUrl value, this led to a regr…

…ession
facebook · Apr 18, 2024 · ab341f2 · ab341f2
1 parent a301c96
commit ab341f2
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 17 deletions.
diff --git a/packages/docusaurus-plugin-content-docs/src/__tests__/index.test.ts b/packages/docusaurus-plugin-content-docs/src/__tests__/index.test.ts
@@ -58,7 +58,7 @@ Available ids are:\n- ${version.docs.map((d) => d.id).join('\n- ')}`,
 }
 
 const createFakeActions = (contentDir: string) => {
-  const routeConfigs: RouteConfig[] = [];
+  let routeConfigs: RouteConfig[] = [];
   const dataContainer: {[key: string]: unknown} = {};
   const globalDataContainer: {pluginName?: {pluginId: unknown}} = {};
 
@@ -83,7 +83,7 @@ const createFakeActions = (contentDir: string) => {
     expectSnapshot: () => {
       // Sort the route config like in src/server/plugins/index.ts for
       // consistent snapshot ordering
-      sortRoutes(routeConfigs, '/');
+      routeConfigs = sortRoutes(routeConfigs, '/');
       expect(routeConfigs).not.toEqual([]);
       expect(routeConfigs).toMatchSnapshot('route config');
       expect(dataContainer).toMatchSnapshot('data');

diff --git a/packages/docusaurus/src/server/plugins/__tests__/routeConfig.test.ts b/packages/docusaurus/src/server/plugins/__tests__/routeConfig.test.ts
@@ -202,9 +202,7 @@ describe('sortRoutes', () => {
       },
     ];
 
-    sortRoutes(routes, '/');
-
-    expect(routes).toMatchSnapshot();
+    expect(sortRoutes(routes, '/')).toMatchSnapshot();
   });
 
   it('sorts route config recursively', () => {
@@ -248,9 +246,7 @@ describe('sortRoutes', () => {
       },
     ];
 
-    sortRoutes(routes, '/');
-
-    expect(routes).toMatchSnapshot();
+    expect(sortRoutes(routes, '/')).toMatchSnapshot();
   });
 
   it('sorts route config given a baseURL', () => {
@@ -290,8 +286,27 @@ describe('sortRoutes', () => {
       },
     ];
 
-    sortRoutes(routes, baseURL);
+    expect(sortRoutes(routes, baseURL)).toMatchSnapshot();
+  });
+
+  it('sorts parent route configs when one included in another', () => {
+    const r1: RouteConfig = {
+      path: '/one',
+      component: '',
+      routes: [{path: `/one/myDoc`, component: ''}],
+    };
+    const r2: RouteConfig = {
+      path: '/',
+      component: '',
+      routes: [{path: `/someDoc`, component: ''}],
+    };
+    const r3: RouteConfig = {
+      path: '/one/another',
+      component: '',
+      routes: [{path: `/one/another/myDoc`, component: ''}],
+    };
 
-    expect(routes).toMatchSnapshot();
+    expect(sortRoutes([r1, r2, r3], '/')).toEqual([r3, r1, r2]);
+    expect(sortRoutes([r3, r1, r2], '/')).toEqual([r3, r1, r2]);
   });
 });
diff --git a/packages/docusaurus/src/server/plugins/plugins.ts b/packages/docusaurus/src/server/plugins/plugins.ts
@@ -232,11 +232,10 @@ function mergeResults({
   plugins: LoadedPlugin[];
   allContentLoadedResult: AllContentLoadedResult;
 }) {
-  const routes: PluginRouteConfig[] = [
-    ...aggregateRoutes(plugins),
-    ...allContentLoadedResult.routes,
-  ];
-  sortRoutes(routes, baseUrl);
+  const routes: PluginRouteConfig[] = sortRoutes(
+    [...aggregateRoutes(plugins), ...allContentLoadedResult.routes],
+    baseUrl,
+  );
 
   const globalData: GlobalData = mergeGlobalData(
     aggregateGlobalData(plugins),

diff --git a/packages/docusaurus/src/server/plugins/routeConfig.ts b/packages/docusaurus/src/server/plugins/routeConfig.ts
@@ -27,7 +27,11 @@ export function applyRouteTrailingSlash<Route extends RouteConfig>(
   };
 }
 
-export function sortRoutes(routeConfigs: RouteConfig[], baseUrl: string): void {
+export function sortRoutes<Route extends RouteConfig>(
+  routesToSort: Route[],
+  baseUrl: string,
+): Route[] {
+  const routeConfigs = [...routesToSort];
   // Sort the route config. This ensures that route with nested
   // routes is always placed last.
   routeConfigs.sort((a, b) => {
@@ -45,6 +49,23 @@ export function sortRoutes(routeConfigs: RouteConfig[], baseUrl: string): void {
     if (!a.routes && b.routes) {
       return -1;
     }
+
+    // If both are parent routes (for example routeBasePath: "/" and "/docs/"
+    // We must order them carefully in case of overlapping paths
+    if (a.routes && b.routes) {
+      if (a.path === b.path) {
+        // We don't really support that kind of routing ATM
+        // React-Router by default will only "enter" a single parent route
+      } else {
+        if (a.path.includes(b.path)) {
+          return -1;
+        }
+        if (b.path.includes(a.path)) {
+          return 1;
+        }
+      }
+    }
+
     // Higher priority get placed first.
     if (a.priority || b.priority) {
       const priorityA = a.priority ?? 0;
@@ -61,7 +82,9 @@ export function sortRoutes(routeConfigs: RouteConfig[], baseUrl: string): void {
 
   routeConfigs.forEach((routeConfig) => {
     if (routeConfig.routes) {
-      sortRoutes(routeConfig.routes, baseUrl);
+      routeConfig.routes = sortRoutes(routeConfig.routes, baseUrl);
     }
   });
+
+  return routeConfigs;
 }