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
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+92-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. */
@@ -40,6 +60,22 @@ function routeFor(path: string): string {
4060 return `/${encodePath(route)}`
4161}
4262
63/**
64 * The document a link was authored in, for a message somebody has to act on.
65 *
66 * Both link failures below name the destination, which is not enough to find
67 * the link: the destination is what is missing, and the file holding it is
68 * what has to be edited. Relative to the documentation root, because an
69 * absolute temporary path is noise in a build log.
70 *
71 * Under an include, `resolveLink` is handed the innermost origin rather than
72 * the host page, so this names the file the link was actually written in.
73 */
74function authoredIn(page: MarkdownPage, root: string): string {
75 const within = relative(root, page.sourceFile)
76 return within && !within.startsWith('..') && !isAbsolute(within) ? within.split(sep).join('/') : page.sourceFile
77}
78
4379function routeKey(path: string): string {
4480 const encoded = path.split('/').map(segment => encodeURIComponent(decodeURIComponent(segment))).join('/')
4581 return encoded.replace(/\/+$/, '') || '/'
@@ -100,13 +136,32 @@ export function createMarkdownPageCatalog(markdownFiles: readonly string[], docs
100136 }
101137 }
102138
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
103158 return Object.freeze({
104159 pages: Object.freeze(pages),
105160 findPage(locale: string, path: string): MarkdownPage | undefined {
106161 const key = `${locale}\0${routeKey(path)}`
107162 return byRoute.get(key) ?? aliases.get(key)
108163 },
109 resolveLink(page: MarkdownPage, destination: string): string {
164 resolveLink(page: MarkdownPage, destination: string, collect?: MarkdownLinkCollector): string {
110165 if (!destination || /^(?:[a-z][a-z\d+.-]*:|\/|#|\?)/i.test(destination))
111166 return destination
112167 const boundary = destination.search(/[?#]/)
@@ -124,16 +179,48 @@ export function createMarkdownPageCatalog(markdownFiles: readonly string[], docs
124179 return destination
125180 const source = sources.get(target)
126181 if (!source)
127 throw new Error(`Markdown link has no documentation source: ${rawPath}`)
182 return refuse(page, rawPath, destination, 'no-documentation-source', collect)
128183 const locale = source.explicitLocale ? source.locale : page.locale
129184 const selected = byPage.get(`${locale}\0${source.path}`)
130185 if (!selected)
131 throw new Error(`Markdown link has no rendered page: ${rawPath}`)
186 return refuse(page, rawPath, destination, 'no-rendered-page', collect)
132187 return `${localizeUrl(language, locale, routeFor(selected.relativePath))}${suffix}`
133188 },
134189 })
135190}
136191
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
137224/** Parse actual anchor attributes; code text, images and external destinations stay intact. */
138225export async function rewriteMarkdownSourceLinks(html: string, context: MarkdownPageContext, origins: MarkdownLinkOrigins = new Map()): Promise<string> {
139226 const stack = [context.page]
@@ -158,7 +245,7 @@ export async function rewriteMarkdownSourceLinks(html: string, context: Markdown
158245 }).on('a[href]', {
159246 element(element) {
160247 const href = decodeHtmlEntities(element.getAttribute('href')!)
161 const destination = context.catalog.resolveLink(stack[stack.length - 1], href)
248 const destination = context.catalog.resolveLink(stack[stack.length - 1], href, context.collectLinkFailures)
162249 if (destination !== href)
163250 element.setAttribute('href', escapeHtmlAttribute(destination))
164251 },
test/markdown-source-links.test.tsmodified+78-0
@@ -4,6 +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, describeLinkFailures, findMarkdownFiles } from '../packages/bunpress/src/markdown-pages'
78import { decodeHtmlEntities, startServer } from '../packages/bunpress/src/serve'
89
910const temporary: string[] = []
@@ -131,6 +132,83 @@ test('a missing linked source refuses rebuild without replacing the previous com
131132 finally { stop() }
132133}, 30000)
133134
135test('a refused link names the document it was authored in', async () => {
136 const f = await fixture()
137 await writeFile(join(f.docs, 'guides/start.md'), '# Start\n\n[gone](../adr/absent.md)\n')
138 const files = await findMarkdownFiles(f.docs)
139 const catalog = createMarkdownPageCatalog(files, f.docs, { enabled: false, locales: ['en'], defaultLocale: 'en' } as never)
140 const page = catalog.findPage('en', '/guides/start')!
141 expect(page).toBeDefined()
142
143 // The destination is the part that is missing, so naming it alone leaves the
144 // reader grepping for whichever of their documents holds the link. The path
145 // is relative to the documentation root: an absolute temporary directory is
146 // noise in a build log.
147 expect(() => catalog.resolveLink(page, '../adr/absent.md')).toThrow('guides/start.md: Markdown link has no documentation source: ../adr/absent.md')
148}, 30000)
149
150test('a refused link inside an include names the included file, not its host', async () => {
151 const f = await fixture()
152 await mkdir(join(f.docs, 'parts'), { recursive: true })
153 await writeFile(join(f.docs, 'parts/snippet.md'), '[gone](./absent.md)\n')
154 const files = await findMarkdownFiles(f.docs)
155 const catalog = createMarkdownPageCatalog(files, f.docs, { enabled: false, locales: ['en'], defaultLocale: 'en' } as never)
156 const included = catalog.findPage('en', '/parts/snippet')!
157
158 // `rewriteMarkdownSourceLinks` hands `resolveLink` the innermost origin on
159 // its include stack, so the file named here is where the link was written
160 // rather than whichever document pulled it in. That is the one a person has
161 // to open, and the two differ exactly when the message matters most.
162 expect(() => catalog.resolveLink(included, './absent.md')).toThrow('parts/snippet.md: Markdown link has no documentation source: ./absent.md')
163}, 30000)
164
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
134212test('canonical output collisions refuse publication', async () => {
135213 const f = await fixture()
136214 await writeFile(join(f.docs, 'guides.md'), '# Ambiguous guides\n')