feat(repl-sdk)!: generate heading ids GitHub's way - #2216
Conversation
`headingId` always kebab-cases the heading text. That is a reasonable default,
but it is not what GitHub generates for the same markdown, so an in-page
`#anchor` can only resolve in one place at a time:
### `setupMirage` kebabCase: #setup-mirage GitHub: #setupmirage
### V2 JSON:API kebabCase: #v2-json-api GitHub: #v2-jsonapi
For docs that are read both on a rendered site and as `.md` files in the repo,
that split is the whole problem -- links authored against one are dead in the
other.
Adds an optional `headingId.slug` to the markdown compiler options. The default
is unchanged, so this is not breaking; consumers who want GitHub-compatible
anchors can pass `github-slugger`:
buildCompiler({ headingId: { slug: (text) => slugger.slug(text) } })
Also normalizes the extracted text before slugging. `extractText` joins a
heading's children with a space on top of the spacing the text nodes already
carry, so `## Hello *there*` extracts as `'Hello there'`. `kebabCase` happens
not to care, but any slugger that maps runs of space to `-` emits
`hello--there` -- github-slugger does exactly that. Normalizing at the boundary
keeps the option usable.
Tests cover the default, a custom slug, the text handed to it, the whitespace
case, and that an explicit `{#custom-id}` still wins.
|
|
Follows the custom-function support in the previous commit with the mode most
consumers actually want, so they don't have to wire up a slugger themselves:
headingId: { slug: 'gfm' }
`'gfm'` produces the anchor GitHub generates for the same markdown, via
github-slugger, which repl-sdk already had in its tree through rehype-slug.
`'kebab'` remains the default and can now be named explicitly.
Two details worth calling out:
- The slugger is created per document, inside the transformer, not in the
plugin factory. github-slugger carries dedupe state (`usage`, `usage-1`), and
a compiler can be reused across documents -- building it once would leak
counts between them. Per document also matches GitHub, where numbering
restarts per file.
- `filterOptions` in compilers/markdown.js only forwarded remarkPlugins and
rehypePlugins, so `headingId` was dropped between the public `md` compiler and
parseMarkdown -- the option would only have worked for callers reaching into
buildCompiler directly. It is now forwarded, with a per-compile value taking
precedence over the compiler-level one.
Documents the option in the repl-sdk README, including the note that heading
anchors are not actually part of the GFM spec -- GitHub generates them in its
rendering layer -- so `'gfm'` is named for the expectation, not the spec.
Adds 9 tests: the named modes, an unknown name throwing, per-document dedupe,
numbering restarting across documents, and the filterOptions forwarding.
Replaces the configurable slugger from the previous commits with the simpler thing @NullVoxPopuli asked for: no option, github-slugger is the behavior. BREAKING CHANGE: heading ids change for any heading whose text isn't already identical under both sluggers -- camelCase and punctuation are where they diverge: ### `setupMirage` before: #setup-mirage after: #setupmirage ### V2 JSON:API before: #v2-json-api after: #v2-jsonapi Links authored against the old ids need updating. Anchors are also now de-duplicated per document (`usage`, `usage-1`), matching GitHub. Drops `change-case`, which was only used for this. Keeps the two pieces that were never about configurability: whitespace is normalized before slugging (`extractText` joins children with a space on top of existing spacing, so `## Hello *there*` would otherwise slug as `hello--there`), and the slugger is built per document rather than per plugin, so its dedupe counter doesn't leak between documents. Updates the snapshot for a heading of `<Hello @foo="two" />`, which moves from `#hello-foo-two` to `#hello-footwo-` -- github-slugger's real output for that text, and a fair illustration of the break.
| } | ||
|
|
||
| /** | ||
| * `extractText` joins a heading's children with a space, on top of whatever |
There was a problem hiding this comment.
what is extractText is that elsewhere in the code?
There was a problem hiding this comment.
It's in this file, ~20 lines up — pre-existing, not something this PR adds. It walks a heading's children and pulls out their text so the slugger has a string to work with.
It did have a bug this PR fixes: it joined the children with a space, so ## Hello *there* came out as 'Hello there' and slugged as hello--there. It concatenates now (11beba2).
Drafted by Claude, reviewed before posting.
|
@NullVoxPopuli rewritten per "gfm should just be the only mode" — no option, no modes, Two things I kept, because neither is configurability:
On breaking-ness — I scanned limber's 119 tracked One existing snapshot did change, which is a fair illustration: a heading of Full blast-radius table is in the description. 27 tests in Drafted by Claude, reviewed before posting. |
| { | ||
| "codeBlocks": [], | ||
| "text": "<h2 id="hello-foo-two"><code><Hello @foo="two" /></code></h2>", | ||
| "text": "<h2 id="hello-footwo-"><code><Hello @foo="two" /></code></h2>", |
There was a problem hiding this comment.
What's this extra dash about?
There was a problem hiding this comment.
That's github-slugger's real output for <Hello @foo="two" /> — the space before the / becomes a dash, and the / itself is stripped, leaving the dash trailing. GitHub produces the same thing for that heading, so I updated the snapshot rather than working around it.
Drafted by Claude, reviewed before posting.
|
ci red |
| }); | ||
|
|
||
| describe('options', () => { | ||
| describe('headingId', () => { |
There was a problem hiding this comment.
this is basically testing GitHub slugger but was useful for me to trust this change
There was a problem hiding this comment.
Agreed it's mostly pinning github-slugger rather than our own logic. I'd keep it: it's the test that fails loudly if the dependency changes behavior or gets swapped out, and it documents the intended mapping for anyone reading later.
Drafted by Claude, reviewed before posting.
| expect(second.text).toBe('<h2 id="usage">Usage</h2>'); | ||
| }); | ||
|
|
||
| it('collapses the whitespace that joining children introduces', async () => { |
There was a problem hiding this comment.
I think this does test our code though right? I wonder if that whitespace normalization is even needed w/ GithubSlugger
There was a problem hiding this comment.
You were right, and it was worse than unnecessary — it made us diverge from GitHub.
You asked whether we could confirm against a reference file in the GitHub UI, so I pushed one to a scratch branch and read it back through GitHub's own renderer (Accept: application/vnd.github.html, which returns the anchored HTML the UI shows):
| Heading | GitHub's anchor |
|---|---|
## Hello *there* |
#hello-there |
## Hello World |
#hello----world |
## setupMirage |
#setupmirage |
## V2 JSON:API |
#v2-jsonapi |
## Usage ×2 |
#usage, #usage-1 |
So GitHub inserts nothing between a heading's children, and it does not collapse whitespace the author wrote. The normalization was doing the second thing, which GitHub doesn't.
The real defect was extractText joining children with a space, adding one the markdown never had. Fixed at the source with join(''); normalizeText is gone (11beba2).
Running that same markdown through parseMarkdown now gives ["hello-there","hello----world","setupmirage","v2-jsonapi","usage","usage-1"] — identical to the GitHub column above.
One consequence worth flagging: this removes the whitespace-collapsing test from #2215. That test encoded kebab-era intent, and it's now asserting the opposite of GitHub's behavior.
Drafted by Claude, reviewed before posting.
My earlier `pnpm install --no-frozen-lockfile` swept in unrelated upgrades. This repo's .npmrc sets `resolution-mode=highest`, so a full install re-resolves every caret range to the newest match -- including `kolay: ^4.1.0`, which moved from 757d6a0e to 2485d3f4 and broke `tutorial#build:test` with `"isCollection" is not exported by kolay`. markdown-it, globby, vite and vitest drifted too. Restored the lockfile to main and hand-applied only the repl-sdk importer delta: change-case out, github-slugger in. 3 lines instead of 323. `github-slugger@2.0.0` was already in the lockfile via rehype-slug, and change-case is still used by ember-repl, so no package entries change. Verified with `pnpm install --frozen-lockfile`.
@gitKrystan asked whether the whitespace normalization was needed at all with github-slugger. It wasn't -- and it was making us diverge from GitHub. Checked against rehype-slug, which is the github-slugger reference: ## Hello World -> id="hello----world" (does NOT collapse) ## Hello *there* -> id="hello-there" (no separator inserted) ## `setupMirage` and more -> id="setupmirage-and-more" So the actual defect was `extractText` joining children with a space, adding one the markdown never had: `## Hello *there*` extracted as `'Hello there'` and slugged as `hello--there`. Joining with '' fixes that at the source. Collapsing author-written runs on top of it was wrong -- GitHub keeps those. Drops normalizeText and the test asserting collapsing (which came from NullVoxPopuli#2215, back when kebabCase was the target). Adds two tests pinning parity with the rehype-slug results above.
Reverting the headingId option left the call chain split across lines; with the argument gone it fits on one again, which is what main had. Caught by repl-sdk#lint:prettier -- I had checked the other files but not this one.
Footnotes
|
| * `'Hello there'` and slugs as `hello--there` rather than `hello-there`. | ||
| * | ||
| * Whitespace the author actually wrote is left alone, since GitHub keeps it too: | ||
| * `## Hello World` slugs as `hello----world` there and here. |
There was a problem hiding this comment.
is this really how we want it to work? 🤔
|
Correcting my earlier comment: the whitespace-normalization bullet no longer holds. That leaves one thing carried over rather than two: the slugger is still built per document, not per plugin, so its dedupe counter doesn't leak between documents. Everything else in that comment still checks out. Re-verified just now against the current branch:
Drafted by Claude, reviewed before posting. |
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Thanks for the verification against GitHub's actual renderer. That table settles "does this match" for good, and gfm-only with no option is the right call.
One structural request: rather than teaching our custom plugin to imitate github-slugger, use rehype-slug and delete most of heading-id.js. rehype-slug is this exact behavior, extracted and maintained upstream: github-slugger, hast-util-to-string extraction, per-document reset, and "skip headings that already have an id", in about 20 lines. #2056 started that swap but lacks the tests and verification you have here, so I'd rather land the swap in this PR and close #2056.
The hand-rolled extractText already diverges from the real extractor in one case:
## *a* *b*The text node between the emphases is whitespace-only, so the !child.value?.trim() branch drops it. We slug ab; GitHub gives a-b. hast-util-to-string keeps that node, so the divergence class disappears with it.
What stays custom is the {#custom-id} handling. It is remark-level, and after remarkRehype the literal {#custom} text is still in the heading, so rehype-slug alone would slug it into the id. Keep a small remark plugin for that piece; rehype-slug skips headings that already carry an id, so the two compose without coordination.
|
@NullVoxPopuli-ai-agent does rehype-slug not have a bug around headings with colons e.g.: ?
me too |
|
No colon bug. Ran <h3 id="v2-jsonapi">V2 JSON:API</h3>Identical to I also ran the other five cases from the description's table plus For history: |
|
Two scope notes for the rehype-slug version, so it's all in one place:
So the end shape is: |
Replaces the hand-rolled extractText. The hand-rolled walk dropped whitespace-only text nodes, so a heading like '## *a* *b*' slugged as 'ab' where GitHub gives 'a-b'. mdast-util-to-string is the maintained extractor with GitHub's semantics, and is already in the dependency tree via remark-gfm, so declaring it installs nothing new. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Withdrawing the rehype-slug request after discussing with @NullVoxPopuli: keeping the plugin as you shaped it (remark-level, github-slugger direct, one place for the Pushed 6eeeca3 to this branch (maintainer edit) with the one piece I'd still not hand-roll: Happy to revert if you'd rather make the change yourself. |
headingId no longer takes options; all heading-id tests now live in one top-level describe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Test-file conflict resolved by keeping the branch's headingId block and
taking main's two {#custom-id} tests in place of the 'leaves alone'
test, which asserted the pre-NullVoxPopuli#2219 skip behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Draft. Rewritten per @NullVoxPopuli's "gfm should just be the only mode — there is no need to provide an option." The configurable version is in the branch history if it's ever wanted back.
What this does
headingIdgenerates ids withgithub-sluggerinstead ofkebabCase. No option, no modes.Why
A
.mdfile is typically read in two places — a rendered site, and the repo on GitHub — and an in-page#anchoronly resolves in both if the two agree on how the id is derived. They didn't.This surfaced in our docs app (kolay → repl-sdk). We had
rehype-slugconfigured and assumed it was doing the work; it wasn't, becauseheadingIdruns first and setsdata.hProperties.id, andrehype-slugskips headings that already carry one. Auditing our corpus: of 127 in-page anchors, 46 matched github-slugger only, 78 matched both, 0 matched kebab-case only — the ids being served were the odd ones out.Any heading whose text isn't already identical under both sluggers changes id. camelCase and punctuation are where they diverge; plain prose headings are unaffected:
## Getting Startedgetting-startedgetting-started## setupMiragesetup-miragesetupmirage## V2 JSON:APIv2-json-apiv2-jsonapi## V0 ActiveRecord Formatv0-active-record-formatv0-activerecord-formatlimber's own docs are unaffected. I scanned all 119 tracked
.mdfiles: 5 in-page anchors, all 5 matching both conventions, 0 that break.One existing test changes, which is a fair illustration of the break — a heading of
`<Hello @foo="two" />`moves from#hello-foo-twoto#hello-footwo-. That trailing dash looks odd, but it's github-slugger's real output for that text, so it's what GitHub would produce too. Snapshot updated rather than worked around.Who should care: any repl-sdk consumer with in-page links authored against the old ids. The tell is camelCase or punctuation in a heading. Downstream, that's kolay sites — which is why I'd suggest this wants a major, though that's your call and I don't know the other consumers.
There's no deprecation path here, since removing the option was the point.
Verified against GitHub itself
@gitKrystan asked whether this could be confirmed with a reference file in the GitHub UI rather than inferred. Pushed one to a scratch branch and read it back through GitHub's own renderer (
Accept: application/vnd.github.html, which returns the anchored HTML the UI shows):## Hello *there*#hello-there#hello-there## Hello World#hello----world#hello----world## setupMirage#setupmirage#setupmirage## V2 JSON:API#v2-jsonapi#v2-jsonapi## Usage×2#usage,#usage-1#usage,#usage-1Identical across all six.
That check also corrected the implementation. An earlier revision normalized whitespace before slugging; GitHub does not collapse runs the author wrote, so normalizing made us diverge. The actual defect was
extractTextjoining a heading's children with a space — adding one the markdown never had, so## Hello *there*extracted as'Hello there'and slugged ashello--there. Fixed at the source by concatenating instead, and the normalization is gone.The other piece kept from the earlier revision: the slugger is built per document, not per plugin. It carries the duplicate-heading counter, and a compiler can be reused across documents, so building it in the factory would leak counts between them. Per document also matches GitHub restarting numbering per file.
change-caseis dropped;heading-id.jswas its only consumer in repl-sdk.Docs
## Usagein the README was empty, so I added a### Heading idssection: the mapping, the dedupe behavior,{#custom-id}precedence, and a note that heading anchors aren't actually part of the GFM spec — GitHub generates them in its rendering layer, andgithub-sluggeris that behavior extracted.Tests
7 in
parse.test.ts: the GitHub-matching id, the colon case, per-document dedupe, numbering restarting across documents, both whitespace cases (nothing inserted between children; author-written runs preserved), and{#custom-id}precedence.This does remove the whitespace-collapsing test added in #2215 — it encoded kebab-era intent and now asserts the opposite of GitHub's behavior.
compilers.ember.onUnhandled.test.tsfails to load andlint:typesreports 3 errors — both identical on cleanmainin my checkout, so unrelated. Prettier clean.🤖 Drafted by Claude (Opus 5) for @gitKrystan. Context: soxhub/auditboard-frontend#42085, where we work around this locally by clearing heading ids before
rehype-slug. If this lands, that workaround gets deleted.