ReviewOS
Reload

also looking at this

stacks/bunpress

fix: name the document a refused Markdown link is in, and report all of them

#95
Open glennmichael123 wants to merge fix/collect-every-refused-link into main
3 files +195 -7

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/bunpress/bin/cli.tsmodified+25-2
@@ -13,7 +13,8 @@ import { generateRssFeed } from '../src/rss'
1313import { buildSearchIndex, SEARCH_INDEX_PATH } from '../src/search-index'
1414import { generateSitemap } from '../src/sitemap'
1515import { siteUrl } from '../src/site-url'
16import { createMarkdownPageCatalog, findMarkdownFiles } from '../src/markdown-pages'
16import type { MarkdownLinkFailure } from '../src/markdown-pages'
17import { createMarkdownPageCatalog, describeLinkFailures, findMarkdownFiles } from '../src/markdown-pages'
1718import { withBuildOutput } from '../src/build-output'
1819import { verifyBuildManifest, writeBuildManifest } from '../src/build-manifest'
1920import { cleanCommand } from './commands/clean'
@@ -224,11 +225,28 @@ export async function buildDocs(options: CliOption = {}): Promise<boolean> {
224225 const catalog = createMarkdownPageCatalog(markdownFiles, docsDir, i18n)
225226 const renderTargets = catalog.pages
226227
228 /*
229 * Every refused link, not the first one.
230 *
231 * Throwing on the first meant a documentation set with N dead links took
232 * N builds to clean up: fix one, rebuild, learn the next. Each run is a
233 * full build, and the count is unknowable until the last one passes.
234 * That is felt most on the first build after adopting the check, when a
235 * set that has never been link-checked is checked all at once.
236 *
237 * Collecting changes nothing about what is published. The throw below
238 * still happens inside `withBuildOutput`, before anything is moved into
239 * place, so a build with a dead link still refuses and still leaves the
240 * previous complete output alone.
241 */
242 const linkFailures: MarkdownLinkFailure[] = []
243 const collectLinkFailures = (failure: MarkdownLinkFailure): void => { linkFailures.push(failure) }
244
227245 for (const target of renderTargets) {
228246 const markdown = await Bun.file(target.sourceFile).text()
229247
230248 // Convert markdown to HTML (handles frontmatter, hero, features, etc.)
231 const { html, frontmatter } = await markdownToHtml(markdown, docsDir, { page: target, catalog })
249 const { html, frontmatter } = await markdownToHtml(markdown, docsDir, { page: target, catalog, collectLinkFailures })
232250
233251 const relativePath = target.relativePath
234252 const currentPath = `/${relativePath}`
@@ -267,6 +285,11 @@ export async function buildDocs(options: CliOption = {}): Promise<boolean> {
267285 }
268286 }
269287
288 // Reported together, and only once every page has had its turn, so the
289 // list is the whole truth rather than the first thing encountered.
290 if (linkFailures.length > 0)
291 throw new Error(describeLinkFailures(linkFailures))
292
270293 if (verbose) {
271294 console.log(`Processed ${markdownFiles.length} markdown files into ${renderTargets.length} pages.`)
272295 }
packages/bunpress/src/markdown-pages.tsmodified+76-5
@@ -13,12 +13,32 @@ export interface MarkdownPage {
1313export interface MarkdownPageCatalog {
1414 readonly pages: readonly MarkdownPage[]
1515 findPage: (locale: string, path: string) => MarkdownPage | undefined
16 resolveLink: (page: MarkdownPage, destination: string) => string
16 resolveLink: (page: MarkdownPage, destination: string, collect?: MarkdownLinkCollector) => string
1717}
1818
19/** A link that named no document, reported rather than thrown. */
20export interface MarkdownLinkFailure {
21 /** The authoring document, relative to the documentation root. */
22 readonly sourceFile: string
23 /** The destination exactly as it was written. */
24 readonly destination: string
25 readonly reason: 'no-documentation-source' | 'no-rendered-page'
26}
27
28export type MarkdownLinkCollector = (failure: MarkdownLinkFailure) => void
29
1930export interface MarkdownPageContext {
2031 readonly page: MarkdownPage
2132 readonly catalog: MarkdownPageCatalog
33 /**
34 * Where to put a refused link instead of throwing on the first one.
35 *
36 * Absent by default, which keeps the single-page callers failing fast: the
37 * dev server renders one page on request and has no later point to report
38 * at, so a throw is the whole answer there. A build renders every page and
39 * can say how many are broken, which is the difference this exists for.
40 */
41 readonly collectLinkFailures?: MarkdownLinkCollector
2242}
2343
2444/** Per-render boundaries for authored content expanded from another file. */
@@ -116,13 +136,32 @@ export function createMarkdownPageCatalog(markdownFiles: readonly string[], docs
116136 }
117137 }
118138
139 /**
140 * Report the link, or throw on it when nobody is collecting.
141 *
142 * Returning the destination untouched is safe precisely because a collecting
143 * caller does not publish: the build throws once it has rendered everything,
144 * so this HTML is discarded. Leaving it alone rather than substituting a
145 * placeholder also keeps the failing page readable while it is inspected.
146 */
147 function refuse(page: MarkdownPage, rawPath: string, destination: string, reason: MarkdownLinkFailure['reason'], collect?: MarkdownLinkCollector): string {
148 const sourceFile = authoredIn(page, root)
149 const message = reason === 'no-documentation-source'
150 ? `${sourceFile}: Markdown link has no documentation source: ${rawPath}`
151 : `${sourceFile}: Markdown link has no rendered page: ${rawPath}`
152 if (!collect)
153 throw new Error(message)
154 collect({ sourceFile, destination: rawPath, reason })
155 return destination
156 }
157
119158 return Object.freeze({
120159 pages: Object.freeze(pages),
121160 findPage(locale: string, path: string): MarkdownPage | undefined {
122161 const key = `${locale}\0${routeKey(path)}`
123162 return byRoute.get(key) ?? aliases.get(key)
124163 },
125 resolveLink(page: MarkdownPage, destination: string): string {
164 resolveLink(page: MarkdownPage, destination: string, collect?: MarkdownLinkCollector): string {
126165 if (!destination || /^(?:[a-z][a-z\d+.-]*:|\/|#|\?)/i.test(destination))
127166 return destination
128167 const boundary = destination.search(/[?#]/)
@@ -140,16 +179,48 @@ export function createMarkdownPageCatalog(markdownFiles: readonly string[], docs
140179 return destination
141180 const source = sources.get(target)
142181 if (!source)
143 throw new Error(`${authoredIn(page, root)}: Markdown link has no documentation source: ${rawPath}`)
182 return refuse(page, rawPath, destination, 'no-documentation-source', collect)
144183 const locale = source.explicitLocale ? source.locale : page.locale
145184 const selected = byPage.get(`${locale}\0${source.path}`)
146185 if (!selected)
147 throw new Error(`${authoredIn(page, root)}: Markdown link has no rendered page: ${rawPath}`)
186 return refuse(page, rawPath, destination, 'no-rendered-page', collect)
148187 return `${localizeUrl(language, locale, routeFor(selected.relativePath))}${suffix}`
149188 },
150189 })
151190}
152191
192/**
193 * One message for every refused link, in the order they were rendered.
194 *
195 * Grouped by document and destination with a count, because the same dead
196 * destination written three times in one file is three identical lines
197 * otherwise. There is no line number: by the time links are resolved the
198 * markdown has already become HTML, so the authored position is gone, and a
199 * fabricated one would be worse than none.
200 */
201export function describeLinkFailures(failures: readonly MarkdownLinkFailure[]): string {
202 const groups = new Map<string, { failure: MarkdownLinkFailure, count: number }>()
203 for (const failure of failures) {
204 const key = `${failure.sourceFile}\0${failure.destination}\0${failure.reason}`
205 const seen = groups.get(key)
206 if (seen)
207 seen.count += 1
208 else
209 groups.set(key, { failure, count: 1 })
210 }
211
212 const lines = [...groups.values()].map(({ failure, count }) => {
213 const reason = failure.reason === 'no-documentation-source' ? 'no documentation source' : 'no rendered page'
214 const times = count > 1 ? ` (${count} times)` : ''
215 return ` ${failure.sourceFile} ${failure.destination} (${reason})${times}`
216 })
217
218 const total = failures.length
219 const heading = total === 1 ? '1 Markdown link could not be resolved:' : `${total} Markdown links could not be resolved:`
220
221 return [heading, ...lines].join('\n')
222}
223
153224/** Parse actual anchor attributes; code text, images and external destinations stay intact. */
154225export async function rewriteMarkdownSourceLinks(html: string, context: MarkdownPageContext, origins: MarkdownLinkOrigins = new Map()): Promise<string> {
155226 const stack = [context.page]
@@ -174,7 +245,7 @@ export async function rewriteMarkdownSourceLinks(html: string, context: Markdown
174245 }).on('a[href]', {
175246 element(element) {
176247 const href = decodeHtmlEntities(element.getAttribute('href')!)
177 const destination = context.catalog.resolveLink(stack[stack.length - 1], href)
248 const destination = context.catalog.resolveLink(stack[stack.length - 1], href, context.collectLinkFailures)
178249 if (destination !== href)
179250 element.setAttribute('href', escapeHtmlAttribute(destination))
180251 },
test/markdown-source-links.test.tsmodified+48-1
@@ -4,7 +4,7 @@ import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'
44import { tmpdir } from 'node:os'
55import { dirname, join } from 'node:path'
66import { buildDocs } from '../packages/bunpress/bin/cli'
7import { createMarkdownPageCatalog, findMarkdownFiles } from '../packages/bunpress/src/markdown-pages'
7import { createMarkdownPageCatalog, describeLinkFailures, findMarkdownFiles } from '../packages/bunpress/src/markdown-pages'
88import { decodeHtmlEntities, startServer } from '../packages/bunpress/src/serve'
99
1010const temporary: string[] = []
@@ -162,6 +162,53 @@ test('a refused link inside an include names the included file, not its host', a
162162 expect(() => catalog.resolveLink(included, './absent.md')).toThrow('parts/snippet.md: Markdown link has no documentation source: ./absent.md')
163163}, 30000)
164164
165test('one build reports every refused link, not the first', async () => {
166 const f = await fixture()
167 await writeFile(join(f.docs, 'guides/start.md'), '# Start\n\n[one](./absent-one.md) and [again](./absent-one.md)\n')
168 await writeFile(join(f.docs, 'guides/second.md'), '# Second\n\n[two](./absent-two.md)\n')
169
170 const reported: string[] = []
171 const error = console.error
172 console.error = (...parts: unknown[]) => { reported.push(parts.map(String).join(' ')) }
173 try {
174 expect(await buildDocs({ dir: f.docs, outdir: f.out, config: f.configFile, searchIndex: false })).toBe(false)
175 }
176 finally { console.error = error }
177
178 // Both documents in a single run. Throwing on the first meant a set with N
179 // dead links took N builds, each one a full build, with no way to know how
180 // many were left until the last passed.
181 const text = reported.join('\n')
182 expect(text).toContain('3 Markdown links could not be resolved')
183 expect(text).toContain('guides/start.md')
184 expect(text).toContain('./absent-one.md')
185 expect(text).toContain('guides/second.md')
186 expect(text).toContain('./absent-two.md')
187}, 30000)
188
189test('a destination repeated in one document is counted rather than repeated', () => {
190 // The real case this came from was three links to one missing file in a
191 // single document. Listed one per line they are three identical rows, since
192 // the authored line number is already gone by the time links resolve.
193 const summary = describeLinkFailures([
194 { sourceFile: 'todo/12-api.md', destination: './09-ci-cd.md', reason: 'no-documentation-source' },
195 { sourceFile: 'todo/12-api.md', destination: './09-ci-cd.md', reason: 'no-documentation-source' },
196 { sourceFile: 'todo/12-api.md', destination: './09-ci-cd.md', reason: 'no-documentation-source' },
197 { sourceFile: 'todo/13-mirror.md', destination: './gone.md', reason: 'no-rendered-page' },
198 ])
199
200 expect(summary).toContain('4 Markdown links could not be resolved')
201 expect(summary).toContain('todo/12-api.md ./09-ci-cd.md (no documentation source) (3 times)')
202 expect(summary).toContain('todo/13-mirror.md ./gone.md (no rendered page)')
203 expect(summary.split('\n')).toHaveLength(3)
204})
205
206test('a single refused link still reads as one', () => {
207 expect(describeLinkFailures([
208 { sourceFile: 'guides/start.md', destination: './absent.md', reason: 'no-documentation-source' },
209 ])).toStartWith('1 Markdown link could not be resolved:')
210})
211
165212test('canonical output collisions refuse publication', async () => {
166213 const f = await fixture()
167214 await writeFile(join(f.docs, 'guides.md'), '# Ambiguous guides\n')