Skip to content

Commit

Permalink
fix(core): fix codegen routesChunkName possible hash collision (faceb…
Browse files Browse the repository at this point in the history
  • Loading branch information
slorber authored Nov 29, 2024
1 parent 1777b14 commit 8098741
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,18 @@
* LICENSE file in the root directory of this source tree.
*/

import {fromPartial} from '@total-typescript/shoehorn';
import {
generateRoutesCode,
genChunkName,
generateRoutePropFilename,
} from '../codegenRoutes';
import type {RouteConfig} from '@docusaurus/types';

function route(routeConfig: Partial<RouteConfig>): RouteConfig {
return fromPartial(routeConfig);
}

describe('generateRoutePropFilename', () => {
it('generate filename based on route path', () => {
expect(
Expand Down Expand Up @@ -206,9 +211,7 @@ describe('loadRoutes', () => {
},
],
};
expect(
generateRoutesCode([nestedRouteConfig], '/', 'ignore'),
).toMatchSnapshot();
expect(generateRoutesCode([nestedRouteConfig])).toMatchSnapshot();
});

it('loads flat route config', () => {
Expand Down Expand Up @@ -243,17 +246,15 @@ describe('loadRoutes', () => {
],
},
};
expect(
generateRoutesCode([flatRouteConfig], '/', 'ignore'),
).toMatchSnapshot();
expect(generateRoutesCode([flatRouteConfig])).toMatchSnapshot();
});

it('rejects invalid route config', () => {
const routeConfigWithoutPath = {
component: 'hello/world.js',
} as RouteConfig;

expect(() => generateRoutesCode([routeConfigWithoutPath], '/', 'ignore'))
expect(() => generateRoutesCode([routeConfigWithoutPath]))
.toThrowErrorMatchingInlineSnapshot(`
"Invalid route config: path must be a string and component is required.
{"component":"hello/world.js"}"
Expand All @@ -263,9 +264,8 @@ describe('loadRoutes', () => {
path: '/hello/world',
} as RouteConfig;

expect(() =>
generateRoutesCode([routeConfigWithoutComponent], '/', 'ignore'),
).toThrowErrorMatchingInlineSnapshot(`
expect(() => generateRoutesCode([routeConfigWithoutComponent]))
.toThrowErrorMatchingInlineSnapshot(`
"Invalid route config: path must be a string and component is required.
{"path":"/hello/world"}"
`);
Expand All @@ -277,6 +277,56 @@ describe('loadRoutes', () => {
component: 'hello/world.js',
} as RouteConfig;

expect(generateRoutesCode([routeConfig], '/', 'ignore')).toMatchSnapshot();
expect(generateRoutesCode([routeConfig])).toMatchSnapshot();
});

it('generates an entry for each route and handle hash collisions', () => {
// See bug https://github.com/facebook/docusaurus/issues/10718#issuecomment-2507635907
const routeConfigs = [
route({
path: '/docs',
component: '@theme/Root',
routes: [
route({
path: '/docs',
component: '@theme/Version',
children: [],
}),
],
}),
route({
path: '/docs',
component: '@theme/Root',
routes: [
route({
path: '/docs',
component: '@theme/Version',
children: [],
}),
],
}),
];

const result = generateRoutesCode(routeConfigs);

// We absolutely want to have 2 entries here, even if routes are the same
// One should not override the other
expect(Object.keys(result.routesChunkNames)).toHaveLength(4);
expect(result.routesChunkNames).toMatchInlineSnapshot(`
{
"/docs-611": {
"__comp": "__comp---theme-version-6-f-8-19f",
},
"/docs-96a": {
"__comp": "__comp---theme-root-1-dd-d3a",
},
"/docs-d3d": {
"__comp": "__comp---theme-version-6-f-8-19f",
},
"/docs-e4f": {
"__comp": "__comp---theme-root-1-dd-d3a",
},
}
`);
});
});
48 changes: 43 additions & 5 deletions packages/docusaurus/src/server/codegen/codegenRoutes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,12 @@ function genChunkNames(
* config node, it returns the node's serialized form, and mutates `registry`,
* `routesPaths`, and `routesChunkNames` accordingly.
*/
function genRouteCode(routeConfig: RouteConfig, res: RoutesCode): string {
function genRouteCode(
routeConfig: RouteConfig,
res: RoutesCode,
index: number,
level: number,
): string {
const {
path: routePath,
component,
Expand All @@ -216,8 +221,39 @@ ${JSON.stringify(routeConfig)}`,
);
}

const routeHash = simpleHash(JSON.stringify(routeConfig), 3);
res.routesChunkNames[`${routePath}-${routeHash}`] = {
// Because 2 routes with the same path could lead to hash collisions
// See https://github.com/facebook/docusaurus/issues/10718#issuecomment-2498516394
function generateUniqueRouteKey(): {
routeKey: string;
routeHash: string;
} {
const hashes = [
// // OG algo to keep former snapshots
() => simpleHash(JSON.stringify(routeConfig), 3),
// Other attempts, not ideal but good enough
// Technically we could use Math.random() here but it's annoying for tests
() => simpleHash(`${level}${index}`, 3),
() => simpleHash(JSON.stringify(routeConfig), 4),
() => simpleHash(`${level}${index}`, 4),
];

for (const tryHash of hashes) {
const routeHash = tryHash();
const routeKey = `${routePath}-${routeHash}`;
if (!res.routesChunkNames[routeKey]) {
return {routeKey, routeHash};
}
}
throw new Error(
`Docusaurus couldn't generate a unique hash for route ${routeConfig.path} (level=${level} - index=${index}).
This is a bug, please report it here!
https://github.com/facebook/docusaurus/issues/10718`,
);
}

const {routeKey, routeHash} = generateUniqueRouteKey();

res.routesChunkNames[routeKey] = {
// Avoid clash with a prop called "component"
...genChunkNames({__comp: component}, 'component', component, res),
...(context &&
Expand All @@ -228,7 +264,9 @@ ${JSON.stringify(routeConfig)}`,
return serializeRouteConfig({
routePath: routePath.replace(/'/g, "\\'"),
routeHash,
subroutesCodeStrings: subroutes?.map((r) => genRouteCode(r, res)),
subroutesCodeStrings: subroutes?.map((r, i) =>
genRouteCode(r, res, i, level + 1),
),
exact,
attributes,
});
Expand All @@ -253,7 +291,7 @@ export function generateRoutesCode(routeConfigs: RouteConfig[]): RoutesCode {

// `genRouteCode` would mutate `res`
const routeConfigSerialized = routeConfigs
.map((r) => genRouteCode(r, res))
.map((r, i) => genRouteCode(r, res, i, 0))
.join(',\n');

res.routesConfig = `import React from 'react';
Expand Down

0 comments on commit 8098741

Please sign in to comment.