Follow up to @mcmire's review comment on #9913: #9913 (comment)
ideally, generate.ts shouldn't know about the exact strategy being used; its goal should be to generate data that can be used to create pages in the documentation site. I think each strategy should be more self-contained; there should be a file or a directory (whatever is more appropriate) that holds all the code for that strategy.
It was deferred as a larger refactor. #9990 did the first step (deleting discovery.ts and the hand-rolled file walking); this is the rest.
The seam is not where the comment suggests
Tracing every export of extraction.ts on main today:
| export |
only caller |
extractFromSourceFile |
scan (via generate.ts) |
classifyMessengerCapabilityTypeDeclaration |
scan and root-messenger |
extractFromMessengerCapabilityTypeDeclaration |
root-messenger |
So extraction.ts (966 lines) is not the shared extractor its name implies. It is a scan-specific front end welded onto a genuinely shared core. That front end is a self-contained subgraph:
extractFromSourceFile
-> findMessengerTypeAliases
-> findAllMessengerCapabilityTypeDeclarations
-> recursivelyFindMessengerCapabilityTypeDeclarations (recursive)
-> classifyMessengerCapabilityTypeDeclaration [shared core]
Its only reference into the shared core is classifyMessengerCapabilityTypeDeclaration, which is exactly the import root-messenger-discovery.ts already makes. That symmetry is good evidence the split belongs there. Fixing only generate.ts, as the comment literally asks, would move the scan internals one file over and leave extraction.ts mixing the same two concerns.
Suggested shape
src/
cli.ts args, site setup, build/serve
generate.ts pipeline only: dedupe, group, write ~350 (from 710)
strategies/
scan.ts globs + *Messenger walking
root-messenger.ts today's root-messenger-discovery.ts
capability-extraction.ts declaration -> packet, shared ~700 (from 966)
ts-project.ts
markdown.ts
types.ts
Each strategy exports one collection function returning MessengerCapabilityPacket[] and owns its own diagnostics, so generate.ts receives packets and nothing else.
Worth avoiding
- A
DiscoveryStrategy interface or factories. Two implementors and one call site; the discriminated union already on GenerateOptions reads better and gives exhaustiveness checking for free.
- Forcing both strategies to return an identical
{ packets, skipped } shape. scan fails per file, root-messenger per capability, so a shared shape would make scan return permanently empty arrays.
Verifying it
The risk is all in the test split (extraction.test.ts is 2452 lines, generate.test.ts 1329). Unit tests alone are not enough here, since most files contain no messenger types and so a wrongly included or excluded file never shows up in the output. What caught two real bugs in #9990 was comparing the exact list of files collected before and after, then diffing full generated docs trees. Both are worth repeating, along with the two clients via --strategy root-messenger.
Follow up to @mcmire's review comment on #9913: #9913 (comment)
It was deferred as a larger refactor. #9990 did the first step (deleting
discovery.tsand the hand-rolled file walking); this is the rest.The seam is not where the comment suggests
Tracing every export of
extraction.tsonmaintoday:extractFromSourceFilescan(viagenerate.ts)classifyMessengerCapabilityTypeDeclarationscanandroot-messengerextractFromMessengerCapabilityTypeDeclarationroot-messengerSo
extraction.ts(966 lines) is not the shared extractor its name implies. It is a scan-specific front end welded onto a genuinely shared core. That front end is a self-contained subgraph:Its only reference into the shared core is
classifyMessengerCapabilityTypeDeclaration, which is exactly the importroot-messenger-discovery.tsalready makes. That symmetry is good evidence the split belongs there. Fixing onlygenerate.ts, as the comment literally asks, would move the scan internals one file over and leaveextraction.tsmixing the same two concerns.Suggested shape
Each strategy exports one collection function returning
MessengerCapabilityPacket[]and owns its own diagnostics, sogenerate.tsreceives packets and nothing else.Worth avoiding
DiscoveryStrategyinterface or factories. Two implementors and one call site; the discriminated union already onGenerateOptionsreads better and gives exhaustiveness checking for free.{ packets, skipped }shape.scanfails per file,root-messengerper capability, so a shared shape would makescanreturn permanently empty arrays.Verifying it
The risk is all in the test split (
extraction.test.tsis 2452 lines,generate.test.ts1329). Unit tests alone are not enough here, since most files contain no messenger types and so a wrongly included or excluded file never shows up in the output. What caught two real bugs in #9990 was comparing the exact list of files collected before and after, then diffing full generated docs trees. Both are worth repeating, along with the two clients via--strategy root-messenger.