From 6ac220cb84b5c833bf04613e78b1fcfa5439239b Mon Sep 17 00:00:00 2001 From: sebastienlorber Date: Mon, 22 Jan 2024 10:16:56 +0100 Subject: [PATCH 1/4] refactor isPathBrokenLink algo to simpler version --- packages/docusaurus/src/server/brokenLinks.ts | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/docusaurus/src/server/brokenLinks.ts b/packages/docusaurus/src/server/brokenLinks.ts index b0b8ae77fd58..339e5d5ec3e4 100644 --- a/packages/docusaurus/src/server/brokenLinks.ts +++ b/packages/docusaurus/src/server/brokenLinks.ts @@ -42,19 +42,20 @@ function getBrokenLinksForPage({ function isPathBrokenLink(linkPath: URLPath) { const pathnames = [linkPath.pathname, decodeURI(linkPath.pathname)]; - const matchedRoutes = pathnames - // @ts-expect-error: React router types RouteConfig with an actual React - // component, but we load route components with string paths. - // We don't actually access component here, so it's fine. - .map((l) => matchRoutes(routes, l)) - .flat(); - // The link path is broken if: - // - it doesn't match any route - // - it doesn't match any collected path - return ( - matchedRoutes.length === 0 && - !pathnames.some((p) => allCollectedPaths.has(p)) - ); + + // A link that matches an existing collected path is valid + if (pathnames.some((p) => allCollectedPaths.has(p))) { + return false; + } + + // @ts-expect-error: React router types RouteConfig with an actual React + // component, but we load route components with string paths. + // We don't actually access component here, so it's fine. + if (pathnames.some((p) => matchRoutes(routes, p).length > 0)) { + return false; + } + + return true; } function isAnchorBrokenLink(linkPath: URLPath) { From 6c314b0739e2270932f9fce12163bd5a9d5fd286 Mon Sep 17 00:00:00 2001 From: sebastienlorber Date: Mon, 22 Jan 2024 13:03:15 +0100 Subject: [PATCH 2/4] Optimize broken links algorithm --- .../src/server/__tests__/brokenLinks.test.ts | 59 ++++++++ packages/docusaurus/src/server/brokenLinks.ts | 127 +++++++++++------- 2 files changed, 139 insertions(+), 47 deletions(-) diff --git a/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts b/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts index 9f98a459f397..df7f6358323a 100644 --- a/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts +++ b/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts @@ -718,4 +718,63 @@ describe('handleBrokenLinks', () => { " `); }); + + it('is fast enough', async () => { + const scale = 2000; + + const routes: SimpleRoute[] = [ + ...Array.from({length: scale}).fill({ + path: '/pageCollected', + }), + ...Array.from({length: scale}).fill({ + path: '/pageUncollected', + }), + ...Array.from({length: scale}).fill({ + path: '/pageDynamic1/:subpath1', + }), + ...Array.from({length: scale}).fill({ + path: '/pageDynamic2/:subpath2', + }), + ]; + + const collectedLinks: Params['collectedLinks'] = { + '/pageCollected': { + links: ['/pageUncollected'], + anchors: ['pageCollectedAnchor'], + }, + }; + + Array.from({length: scale}).forEach((_, i) => { + collectedLinks[`/pageCollected${i}`] = { + links: [ + '/pageCollected', + '/pageUncollected', + ...Array.from({length: scale}).fill( + '/pageCollected#pageCollectedAnchor', + ), + ...Array.from({length: scale}).fill( + `/pageCollected?age=${i}`, + ), + + // We keep those static (not using "i") + // because we can't optimize dynamic links + '/pageDynamic1/staticSubPath1', + '/pageDynamic2/staticSubPath2', + ], + anchors: ['anyAnchor'], + }; + }); + + const timeBefore = Date.now(); + await testBrokenLinks({ + routes, + collectedLinks, + }); + const timeAfter = Date.now(); + + // Not sure if it's super elegant but we tst for JS execution time here + // See https://twitter.com/sebastienlorber/status/1749392773415858587 + // On Mac M1 execution changed from 10s to 200ms after my optimizations + expect(timeAfter - timeBefore).toBeLessThan(3000); + }); }); diff --git a/packages/docusaurus/src/server/brokenLinks.ts b/packages/docusaurus/src/server/brokenLinks.ts index 339e5d5ec3e4..31641c383574 100644 --- a/packages/docusaurus/src/server/brokenLinks.ts +++ b/packages/docusaurus/src/server/brokenLinks.ts @@ -7,11 +7,18 @@ import _ from 'lodash'; import logger from '@docusaurus/logger'; -import {matchRoutes} from 'react-router-config'; +import {matchRoutes as reactRouterMatchRoutes} from 'react-router-config'; import {parseURLPath, serializeURLPath, type URLPath} from '@docusaurus/utils'; import {getAllFinalRoutes} from './utils'; import type {RouteConfig, ReportingSeverity} from '@docusaurus/types'; +function matchRoutes(routeConfig: RouteConfig[], pathname: string) { + // @ts-expect-error: React router types RouteConfig with an actual React + // component, but we load route components with string paths. + // We don't actually access component here, so it's fine. + return reactRouterMatchRoutes(routeConfig, pathname); +} + type BrokenLink = { link: string; resolvedLink: string; @@ -26,89 +33,113 @@ type CollectedLinks = { [pathname: string]: {links: string[]; anchors: string[]}; }; -function getBrokenLinksForPage({ +type BrokenLinksHelper = { + collectedLinks: CollectedLinks; + isPathBrokenLink: (linkPath: URLPath) => boolean; + isAnchorBrokenLink: (linkPath: URLPath) => boolean; +}; + +function createBrokenLinksHelper({ collectedLinks, - pagePath, - pageLinks, routes, }: { collectedLinks: CollectedLinks; - pagePath: string; - pageLinks: string[]; - pageAnchors: string[]; routes: RouteConfig[]; -}): BrokenLink[] { - const allCollectedPaths = new Set(Object.keys(collectedLinks)); +}): BrokenLinksHelper { + const validPathnames = new Set(Object.keys(collectedLinks)); + + // Matching against the route array can be expensive + // If the route is already in the valid pathnames, + // we can avoid matching against it as an optimization + const remainingRoutes = routes.filter( + (route) => !validPathnames.has(route.path), + ); + + function isPathnameMatchingAnyRoute(pathname: string): boolean { + if (matchRoutes(remainingRoutes, pathname).length > 0) { + // IMPORTANT: this is an optimization here + // See https://github.com/facebook/docusaurus/issues/9754 + // Large Docusaurus sites have many routes! + // We try to minimize calls to a possibly expensive matchRoutes function + validPathnames.add(pathname); + return true; + } + + return false; + } function isPathBrokenLink(linkPath: URLPath) { const pathnames = [linkPath.pathname, decodeURI(linkPath.pathname)]; - - // A link that matches an existing collected path is valid - if (pathnames.some((p) => allCollectedPaths.has(p))) { + if (pathnames.some((p) => validPathnames.has(p))) { return false; } - - // @ts-expect-error: React router types RouteConfig with an actual React - // component, but we load route components with string paths. - // We don't actually access component here, so it's fine. - if (pathnames.some((p) => matchRoutes(routes, p).length > 0)) { + if (pathnames.some(isPathnameMatchingAnyRoute)) { return false; } - return true; } function isAnchorBrokenLink(linkPath: URLPath) { const {pathname, hash} = linkPath; - // Link has no hash: it can't be a broken anchor link if (hash === undefined) { return false; } - // Link has empty hash ("#", "/page#"...): we do not report it as broken // Empty hashes are used for various weird reasons, by us and other users... // See for example: https://github.com/facebook/docusaurus/pull/6003 if (hash === '') { return false; } - const targetPage = collectedLinks[pathname] || collectedLinks[decodeURI(pathname)]; - // link with anchor to a page that does not exist (or did not collect any // link/anchor) is considered as a broken anchor if (!targetPage) { return true; } - // it's a broken anchor if the target page exists // but the anchor does not exist on that page const hashes = [hash, decodeURIComponent(hash)]; return !targetPage.anchors.some((anchor) => hashes.includes(anchor)); } - const brokenLinks = pageLinks.flatMap((link) => { + return { + collectedLinks, + isPathBrokenLink, + isAnchorBrokenLink, + }; +} + +function getBrokenLinksForPage({ + pagePath, + helper, +}: { + pagePath: string; + helper: BrokenLinksHelper; +}): BrokenLink[] { + const pageData = helper.collectedLinks[pagePath]!; + + // If a link is present twice in page, it's not useful to process it twice + const uniqueLinks = _.uniq(pageData.links); + + const brokenLinks: BrokenLink[] = []; + + uniqueLinks.forEach((link) => { const linkPath = parseURLPath(link, pagePath); - if (isPathBrokenLink(linkPath)) { - return [ - { - link, - resolvedLink: serializeURLPath(linkPath), - anchor: false, - }, - ]; - } - if (isAnchorBrokenLink(linkPath)) { - return [ - { - link, - resolvedLink: serializeURLPath(linkPath), - anchor: true, - }, - ]; + if (helper.isPathBrokenLink(linkPath)) { + brokenLinks.push({ + link, + resolvedLink: serializeURLPath(linkPath), + anchor: false, + }); + } else if (helper.isAnchorBrokenLink(linkPath)) { + brokenLinks.push({ + link, + resolvedLink: serializeURLPath(linkPath), + anchor: true, + }); } - return []; }); return brokenLinks; @@ -134,14 +165,16 @@ function getBrokenLinks({ }): BrokenLinksMap { const filteredRoutes = filterIntermediateRoutes(routes); - return _.mapValues(collectedLinks, (pageCollectedData, pagePath) => { + const helper = createBrokenLinksHelper({ + collectedLinks, + routes: filteredRoutes, + }); + + return _.mapValues(collectedLinks, (_unused, pagePath) => { try { return getBrokenLinksForPage({ - collectedLinks, - pageLinks: pageCollectedData.links, - pageAnchors: pageCollectedData.anchors, pagePath, - routes: filteredRoutes, + helper, }); } catch (e) { throw new Error(`Unable to get broken links for page ${pagePath}.`, { From b6250f77c8098bc39d120a1be6030ab99eb9f597 Mon Sep 17 00:00:00 2001 From: sebastienlorber Date: Mon, 22 Jan 2024 18:35:05 +0100 Subject: [PATCH 3/4] Optimize broken links checker --- .../src/server/__tests__/brokenLinks.test.ts | 86 +++++++++---------- packages/docusaurus/src/server/brokenLinks.ts | 63 +++++++++----- 2 files changed, 82 insertions(+), 67 deletions(-) diff --git a/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts b/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts index df7f6358323a..d5cc06a8b2d0 100644 --- a/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts +++ b/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts @@ -6,6 +6,7 @@ */ import {jest} from '@jest/globals'; +import reactRouterConfig from 'react-router-config'; import {handleBrokenLinks} from '../brokenLinks'; import type {RouteConfig} from '@docusaurus/types'; @@ -719,62 +720,59 @@ describe('handleBrokenLinks', () => { `); }); - it('is fast enough', async () => { - const scale = 2000; + it('is performant and minimize calls to matchRoutes', async () => { + const matchRoutesMock = jest.spyOn(reactRouterConfig, 'matchRoutes'); + + const scale = 100; const routes: SimpleRoute[] = [ + ...Array.from({length: scale}).map((_, i) => ({ + path: `/page${i}`, + })), ...Array.from({length: scale}).fill({ - path: '/pageCollected', - }), - ...Array.from({length: scale}).fill({ - path: '/pageUncollected', - }), - ...Array.from({length: scale}).fill({ - path: '/pageDynamic1/:subpath1', - }), - ...Array.from({length: scale}).fill({ - path: '/pageDynamic2/:subpath2', + path: '/pageDynamic/:subpath1', }), ]; - const collectedLinks: Params['collectedLinks'] = { - '/pageCollected': { - links: ['/pageUncollected'], - anchors: ['pageCollectedAnchor'], - }, - }; - - Array.from({length: scale}).forEach((_, i) => { - collectedLinks[`/pageCollected${i}`] = { - links: [ - '/pageCollected', - '/pageUncollected', - ...Array.from({length: scale}).fill( - '/pageCollected#pageCollectedAnchor', - ), - ...Array.from({length: scale}).fill( - `/pageCollected?age=${i}`, - ), - - // We keep those static (not using "i") - // because we can't optimize dynamic links - '/pageDynamic1/staticSubPath1', - '/pageDynamic2/staticSubPath2', - ], - anchors: ['anyAnchor'], - }; - }); + const collectedLinks: Params['collectedLinks'] = Object.fromEntries( + Array.from({length: scale}).map((_, i) => [ + `/page${i}`, + { + links: [ + ...Array.from({length: scale}).flatMap((_2, j) => [ + `/page${j}`, + `/page${j}?age=42`, + `/page${j}#anchor${j}`, + `/page${j}?age=42#anchor${j}`, + `/pageDynamic/subPath${j}`, + `/pageDynamic/subPath${j}?age=42`, + // `/pageDynamic/subPath${j}#anchor${j}`, + // `/pageDynamic/subPath${j}?age=42#anchor${j}`, + ]), + ], + anchors: Array.from({length: scale}).map( + (_2, j) => `anchor${j}`, + ), + }, + ]), + ); - const timeBefore = Date.now(); + // console.time('testBrokenLinks'); await testBrokenLinks({ routes, collectedLinks, }); - const timeAfter = Date.now(); + // console.timeEnd('testBrokenLinks'); - // Not sure if it's super elegant but we tst for JS execution time here + // Idiomatic code calling matchRoutes multiple times is not performant + // We try to minimize the calls to this expensive function + // Otherwise large sites will have super long execution times + // See https://github.com/facebook/docusaurus/issues/9754 // See https://twitter.com/sebastienlorber/status/1749392773415858587 - // On Mac M1 execution changed from 10s to 200ms after my optimizations - expect(timeAfter - timeBefore).toBeLessThan(3000); + // We expect no more matchRoutes calls than number of dynamic route links + expect(matchRoutesMock).toHaveBeenCalledTimes(scale); + // We expect matchRoutes to be called with a reduced number of routes + expect(routes).toHaveLength(scale * 2); + expect(matchRoutesMock.mock.calls[0]![0]).toHaveLength(scale); }); }); diff --git a/packages/docusaurus/src/server/brokenLinks.ts b/packages/docusaurus/src/server/brokenLinks.ts index 31641c383574..3590b71351b1 100644 --- a/packages/docusaurus/src/server/brokenLinks.ts +++ b/packages/docusaurus/src/server/brokenLinks.ts @@ -33,8 +33,15 @@ type CollectedLinks = { [pathname: string]: {links: string[]; anchors: string[]}; }; +// We use efficient data structures for performance reasons +// See https://github.com/facebook/docusaurus/issues/9754 +type CollectedLinksNormalized = Map< + string, + {links: Set; anchors: Set} +>; + type BrokenLinksHelper = { - collectedLinks: CollectedLinks; + collectedLinks: CollectedLinksNormalized; isPathBrokenLink: (linkPath: URLPath) => boolean; isAnchorBrokenLink: (linkPath: URLPath) => boolean; }; @@ -43,10 +50,10 @@ function createBrokenLinksHelper({ collectedLinks, routes, }: { - collectedLinks: CollectedLinks; + collectedLinks: CollectedLinksNormalized; routes: RouteConfig[]; }): BrokenLinksHelper { - const validPathnames = new Set(Object.keys(collectedLinks)); + const validPathnames = new Set(collectedLinks.keys()); // Matching against the route array can be expensive // If the route is already in the valid pathnames, @@ -92,16 +99,20 @@ function createBrokenLinksHelper({ return false; } const targetPage = - collectedLinks[pathname] || collectedLinks[decodeURI(pathname)]; + collectedLinks.get(pathname) || collectedLinks.get(decodeURI(pathname)); // link with anchor to a page that does not exist (or did not collect any // link/anchor) is considered as a broken anchor if (!targetPage) { return true; } - // it's a broken anchor if the target page exists - // but the anchor does not exist on that page - const hashes = [hash, decodeURIComponent(hash)]; - return !targetPage.anchors.some((anchor) => hashes.includes(anchor)); + // it's a not broken anchor if the anchor exists on the target page + if ( + targetPage.anchors.has(hash) || + targetPage.anchors.has(decodeURIComponent(hash)) + ) { + return false; + } + return true; } return { @@ -118,14 +129,11 @@ function getBrokenLinksForPage({ pagePath: string; helper: BrokenLinksHelper; }): BrokenLink[] { - const pageData = helper.collectedLinks[pagePath]!; - - // If a link is present twice in page, it's not useful to process it twice - const uniqueLinks = _.uniq(pageData.links); + const pageData = helper.collectedLinks.get(pagePath)!; const brokenLinks: BrokenLink[] = []; - uniqueLinks.forEach((link) => { + pageData.links.forEach((link) => { const linkPath = parseURLPath(link, pagePath); if (helper.isPathBrokenLink(linkPath)) { brokenLinks.push({ @@ -160,7 +168,7 @@ function getBrokenLinks({ collectedLinks, routes, }: { - collectedLinks: CollectedLinks; + collectedLinks: CollectedLinksNormalized; routes: RouteConfig[]; }): BrokenLinksMap { const filteredRoutes = filterIntermediateRoutes(routes); @@ -170,9 +178,10 @@ function getBrokenLinks({ routes: filteredRoutes, }); - return _.mapValues(collectedLinks, (_unused, pagePath) => { + const result: BrokenLinksMap = {}; + collectedLinks.forEach((_unused, pagePath) => { try { - return getBrokenLinksForPage({ + result[pagePath] = getBrokenLinksForPage({ pagePath, helper, }); @@ -182,6 +191,7 @@ function getBrokenLinks({ }); } }); + return result; } function brokenLinkMessage(brokenLink: BrokenLink): string { @@ -337,15 +347,22 @@ function reportBrokenLinks({ // JS users might call "collectLink(undefined)" for example // TS users might call "collectAnchor('#hash')" with/without # // We clean/normalize the collected data to avoid obscure errors being thrown +// We also use optimized data structures for a faster algorithm function normalizeCollectedLinks( collectedLinks: CollectedLinks, -): CollectedLinks { - return _.mapValues(collectedLinks, (pageCollectedData) => ({ - links: pageCollectedData.links.filter(_.isString), - anchors: pageCollectedData.anchors - .filter(_.isString) - .map((anchor) => (anchor.startsWith('#') ? anchor.slice(1) : anchor)), - })); +): CollectedLinksNormalized { + const result: CollectedLinksNormalized = new Map(); + Object.entries(collectedLinks).forEach(([pathname, pageCollectedData]) => { + result.set(pathname, { + links: new Set(pageCollectedData.links.filter(_.isString)), + anchors: new Set( + pageCollectedData.anchors + .filter(_.isString) + .map((anchor) => (anchor.startsWith('#') ? anchor.slice(1) : anchor)), + ), + }); + }); + return result; } export async function handleBrokenLinks({ From 3e750bbab0a8eb2394c1910210b505a3758251ff Mon Sep 17 00:00:00 2001 From: slorber Date: Mon, 22 Jan 2024 17:49:07 +0000 Subject: [PATCH 4/4] refactor: apply lint autofix --- .../src/server/__tests__/brokenLinks.test.ts | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts b/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts index d5cc06a8b2d0..5619b1f7ba34 100644 --- a/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts +++ b/packages/docusaurus/src/server/__tests__/brokenLinks.test.ts @@ -736,25 +736,25 @@ describe('handleBrokenLinks', () => { const collectedLinks: Params['collectedLinks'] = Object.fromEntries( Array.from({length: scale}).map((_, i) => [ - `/page${i}`, - { - links: [ - ...Array.from({length: scale}).flatMap((_2, j) => [ - `/page${j}`, - `/page${j}?age=42`, - `/page${j}#anchor${j}`, - `/page${j}?age=42#anchor${j}`, - `/pageDynamic/subPath${j}`, - `/pageDynamic/subPath${j}?age=42`, - // `/pageDynamic/subPath${j}#anchor${j}`, - // `/pageDynamic/subPath${j}?age=42#anchor${j}`, - ]), - ], - anchors: Array.from({length: scale}).map( - (_2, j) => `anchor${j}`, - ), - }, - ]), + `/page${i}`, + { + links: [ + ...Array.from({length: scale}).flatMap((_2, j) => [ + `/page${j}`, + `/page${j}?age=42`, + `/page${j}#anchor${j}`, + `/page${j}?age=42#anchor${j}`, + `/pageDynamic/subPath${j}`, + `/pageDynamic/subPath${j}?age=42`, + // `/pageDynamic/subPath${j}#anchor${j}`, + // `/pageDynamic/subPath${j}?age=42#anchor${j}`, + ]), + ], + anchors: Array.from({length: scale}).map( + (_2, j) => `anchor${j}`, + ), + }, + ]), ); // console.time('testBrokenLinks');