Repository navigation
feat(platform-api-docs): add root-messenger strategy - #9913
Conversation
8db1696 to
81fb650
Compare
|
@metamaskbot publish-preview |
|
@metamaskbot publish-preview |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5fe1f30. Configure here.
|
@metamaskbot publish-preview |
e514f29 to
3c77cf4
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
There was a problem hiding this comment.
I went through the changes here and made a bunch of comments. I've collected suggestions for most of them in this PR: #9981. Most of the changes are to root-messenger-discovery.ts, to make it easier to understand. I tried to break things up into commits so if you want to cherry-pick anything you can.
The only thing I didn't do was to consolidate the existing "scan strategy" code into one file just like we've done with the "root messenger strategy" code. I figured that was a larger refactor.
I also haven't manually tested this flow yet. I will do that shortly.
## Explanation
<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:
* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->
- Refactor and clean up `cli.ts`
- Update `cli.ts` to extract `parseArguments`, further clean `main`
- Revise `root-messenger-discovery.ts` (update terminology, JSDoc)
- Simplify `{Scan,RootMessenger}StrategyOptions`, enforce that
`generate` takes strategy
## References
<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?
For example:
* Fixes #12345
* Related to #67890
-->
## Checklist
- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md)
- [ ] I've introduced [breaking
changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md)
in this PR and have prepared draft pull requests for clients and
consumer packages to resolve them
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Programmatic callers of `generate` must pass `strategy` (breaking for
library use); root-messenger discovery is heavily renamed but CLI-facing
behavior should stay the same.
>
> **Overview**
> Refactors **platform-api-docs** around clearer typing and naming for
the scan vs **root-messenger** discovery paths, without changing CLI
flags.
>
> **CLI (`cli.ts`)** pulls argument parsing into **`parseArguments`**,
models options as a discriminated union (`ParsedArguments`), and has
**`checkStrategyArgs`** return normalized scan or root-messenger args
instead of only validating. **`--root-actions`** / **`--root-events`**
are coerced via **`parseRootCapabilitiesTypeReference`** at parse time;
**`main`** calls **`generate`** in separate branches with typed options.
>
> **`generate`** now **requires** an explicit **`strategy: 'scan' |
'root-messenger'`** on **`GenerateOptions`** (scan and root-messenger
option shapes are a strict union). Scan tests pass **`strategy:
'scan'`** accordingly.
>
> **Root-messenger discovery** renames and restructures APIs:
**`RootTypeReference`** → **`RootCapabilitiesTypeReference`**,
**`parseRootTypeReference`** → **`parseRootCapabilitiesTypeReference`**,
**`discoverFromRootMessenger`** →
**`discoverFromRootMessengerCapabilitiesTypes`**, with result fields
**`capabilityPackets`** / **`skippedCapabilities`**
(**`unnamedCapabilities`**, **`unextractableCapabilities`**). Internal
helpers use object parameters and expanded JSDoc; extraction behavior is
unchanged.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
4f7745c. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
|
@metamaskbot publish-preview |
mcmire
left a comment
There was a problem hiding this comment.
Some more suggestions. I forgot that in my PR I made an effort to represent --root-actions and --root-events as references to capability collection types rather than unions (because technically neither may be a true union). So most of these changes are to make sure the JSDoc captures that point of view.
I have some more changes to make to the README but I left a comment there about reformatting first.
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@cryptodev-2s I found MetaMask/metamask-extension#40352 and MetaMask/metamask-mobile#26526 and tried them out. On Extension this is what I'm seeing: On Mobile this is what I'm seeing: I wonder if it may be worth it eventually to throw errors instead of printing warnings if we encounter issues. That will motivate us to correct them. Or maybe there's a lint rule or some other check we can add to prevent these problems from creeping into the codebase altogether that would be nice too... Still, this worked great and I can see that there are clear differences between Extension and Mobile (for instance, Extension includes At this point |
…MetaMask#9990) Stacked on MetaMask#9913, targets its branch. Replaces hand-rolled file discovery with `Project#addSourceFilesAtPaths`, which takes glob patterns directly. Drops `discovery.ts`, the per-file add/extract loops, `listTargetSubdirectories`, and the `glob` dependency. Also merges the two `Project` factories (they differed by one field) and removes the redundant `getSourceFile()` guard, closing MetaMask#9913 (comment). Net 254 lines deleted. Generation over this monorepo: **114s to 8.6s**. Output is byte identical. `diff -r` on full docs trees generated before and after, for core, mobile and extension: all empty, sidebars included. ## Two traps, if you touch these patterns Both produced wrong output while the whole suite stayed green. - **Glob roots need `realpath`.** The matcher does not follow a symlinked *ancestor*. Under `/var/folders/...` the declaration glob returned 0 where `/private/var/...` returned 1980, so any project under `/tmp` or `/var` would have emitted empty docs. - **Negations must be anchored.** `!**/*.test.ts` resolves against the process cwd, not against the pattern it accompanies. Since `yarn workspace` sets cwd to the package dir, every exclusion silently stopped matching and test fixtures leaked in (117 namespaces instead of 112). Related: declaration-file exclusions are deliberately narrower than the source ones. A blanket `dist/**` matches the segment those files live under and drops all 1980. ## Behaviour change A broken symlink used to emit a per-file parse warning. The matcher now resolves it away before ts-morph sees it, so it is skipped silently; valid files are still documented. One test rewritten to match. ## Follow-up Second step makes each strategy self contained, which is what MetaMask#9913 (comment) asked for. Worth knowing before that review: `extraction.ts` is a scan-only front end welded onto a shared core, so the split has to cut through it too, not just `generate.ts`. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Internal tooling refactor with byte-identical output claimed; main risk is glob/exclusion edge cases on unusual filesystem layouts, now covered by new tests. > > **Overview** > **Speeds up doc generation** by replacing per-file `glob` discovery and `addSourceFileAtPath` loops with a single shared ts-morph `Project` that bulk-loads matches via `addSourceFilesAtPaths`. Removes `discovery.ts`, the `glob` dependency, and merges the two project factories into shared `createProject()` in `ts-project.ts`. > > The scan strategy now builds **resolved, forward-slashed glob roots** (`toGlobPath` + `realpath`) and **content-root–anchored exclusion negations** so test/dist skips work when cwd ≠ project root and monorepo packages named `test`/`dist` are not excluded wholesale. Added source files are **sorted by path** for stable dedup/source links; extraction uses a **realpath’d project root** for relative source paths. > > **Behavior:** unreadable paths (e.g. broken symlinks) are skipped without per-file parse warnings; valid files still document. Tests cover the `packages/test` exclusion edge case and the symlink case. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 55c4a26. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

Explanation
@metamask/platform-api-docsdocuments the platform API — every messenger action and event a project exposes. Until now it had one way of finding them: parse every TypeScript source and declaration file it can reach (the scan directories,packages/*/src, andnode_modules/@metamask/*/dist/**/*.d.cts) and walk every type alias named*Messenger.That is the right approach for this monorepo, which has no single messenger aggregating every capability. It is a poor fit for a client, which already declares the complete set on its root messenger. Re-deriving that from the whole dependency tree means parsing ~11,600 files in
metamask-mobileand ~4,500 inmetamask-extension, to rediscover something the client has written down in one place.This PR adds a second strategy that reads what the client already declares.
--strategyscan(default) — unchanged behaviour, and the only option for a project with no single aggregating messenger.root-messenger— resolves the two types named by--root-actionsand--root-events(each written<file>#<TypeName>) and lets the TypeScript type checker enumerate them. Only the named files are opened; the checker pulls in the rest.Flags belonging to the strategy that wasn't selected are rejected rather than ignored, via a yargs
.check, so a mistaken invocation fails loudly instead of quietly producing docs built the wrong way. The<file>#<TypeName>references are parsed in a yargs.coerce, so a malformed one is reported like any otherbad argument before work begins.
Why the type checker rather than the AST
This is the non-obvious part. The two clients declare their root unions differently:
metamask-mobilewritesGlobalActionsby hand as a union of type references. A syntactic walk would work.metamask-extensionderivesRootMessengerActionsfrom a registry of messenger factories viaMessengerActions<ReturnType<(typeof MESSENGER_FACTORIES)[…]['getMessenger']>>.There is no syntactic union to walk — only the type checker can say what it contains.
Going through the checker handles both shapes with one code path. Once it reports which capability types are in the union, each declaration is handed to the existing extractor in
extraction.ts, so JSDoc, handler/payload signatures, source links, and deprecation flags come out identical toscan. The new module is a discovery front-end, not a second extractor.Two details worth knowing:
consulted. Missing the interface case silently dropped 61 actions and 8 events on mobile before it was fixed.
type Actions = Foo<Bar>) the checker attributes the alias to the root union itself, which would hand the extractor the wrong declaration. That case is guarded.Failure behaviour
Generation now fails loudly instead of producing an empty site.
writeOutputdeletesdocs/before writing, so a root union that resolves to nothing — a renamed type, or imports that don't resolve — would previously have replaced apublished docs directory with an empty one and exited
0. It now throws, naming both references.Capability types that can't be documented are reported with their names rather than counted, in three buckets: declared inline (no name or JSDoc), unresolved (
any/unknown, usually a failed import), and unextractable (a shape the extractor rejects). A count alone isn't actionable at this scale.Also fixed: MDX escaping
escapeJsDocTextForMdxescaped{and}but not<, which MDX reads as the start of a JSX tag. A@returnscomment such asPromise<PointsBoostDto[]>therefore failed the site build rather than rendering:This is pre-existing and independent of the new strategy —
scanproduces the byte-identical line — but it blocked--buildand--servefor both clients, so it is fixed here. Affects description,@param, and@returnstext; handler and payload signatures were already safe inside fenced code blocks.Benchmarks
Measured on a warm checkout, doc generation only.
scanroot-messengermetamask-extensionmetamask-mobilescanparses ~5,600app/**/*.tsplus ~6,026.d.ctsin mobile, and ~4,472.d.ctsin the extension.root-messengeropens the entry file and lets the checker pull in only what the union references.Strategy comparison
metamask-mobilescanroot-messengerThe 37 not documented are mostly controllers genuinely not on the root messenger —
PasskeyControlleralone accounts for 17, plusRatesController(4), the sample controllers, and the decrypt/encrypt message managers. Nothing is found byroot-messengerthatscanmisses.metamask-extensionscanroot-messenger99 of the 131-capability gap is
PerpsController, and the cause is worth flagging to the extension team rather than treating as a tool limitation:RootMessengerActionsisMessengerActions<ChildMessengers>— the union of what each child messenger is allowed to call, not what each controller provides. A controller whose actions are only invoked from the UI, never from another controller's messenger, never appears.PerpsControlleris registered inMESSENGER_FACTORIESyet contributes zero constituents.root-messengerdocuments exactly what the named types contain. Full coverage in. the extension needs an aggregate of provided actions, which is an extension-side change.Conversely,
root-messengerfinds 6 capabilitiesscanmisses(MultichainRoutingService×4,PPOMController×2).Reported-but-skipped, current run: mobile 39 unextractable; extension 2 inline + 33 unextractable.
Usage in clients
Once published, add the dependency and two scripts. For
metamask-mobile:{ "scripts": { "docs:platform-api:build": "platform-api-docs --build --project-label Mobile --strategy root-messenger --root-actions 'app/core/Engine/types.ts#GlobalActions' --root-events 'app/core/Engine/types.ts#GlobalEvents'", "docs:platform-api:serve": "platform-api-docs --serve --project-label Mobile --site-base-url / --strategy root-messenger --root-actions 'app/core/Engine/types.ts#GlobalActions' --root-events 'app/core/Engine/types.ts#GlobalEvents'" } }For
metamask-extension, the label and references change:Notes for consumers:
#inside quotes — unquoted, most shells treat it as a comment and silently truncate the argument.--root-actions/--root-eventsare relative to the project path, not the shell's working directory.<project-path>/.platform-api-docs; gitignore it.metamask-extensionadditionally needs itspostcss-loader/jitiresolution narrowed topostcss-loader@^8.2.1/jiti. The unversioned form also stubs thepostcss-loader@^7.3.4that@docusaurus/bundlerdepends on, replacingjitiwith an empty package and breaking the site build. Narrowing preserves the stub's original intent for the extension's ownpostcss-loader@8.2.1.References
Fixes: https://consensyssoftware.atlassian.net/browse/WPC-1202
jitiresolution fix): feat: generate and publish Platform API docs metamask-extension#40352Checklist
Note
Low Risk
Changes are confined to the docs CLI and generation pipeline; default
scanbehavior is preserved with an explicitstrategyfield in tests.Overview
Adds a
root-messengerdiscovery path alongside the existingscandefault: the CLI accepts--strategy,--root-actions, and--root-events(<file>#<TypeName>), validates that strategy-specific flags are not mixed, and routes generation through type-checker resolution of the project’s root action/event unions instead of scanning the whole tree.New
root-messenger-discoverywalks those unions (including checker-derived unions likeMessengerActions<…>), reuses the shared extractor inextraction.ts(withclassifyMessengerCapabilityTypeDeclarationexported for reuse), warns on skipped inline or unextractable capabilities, and throws when unions resolve toany/unknownor would produce zero docs so an empty site cannot overwrite published output.escapeJsDocTextForMdxnow escapes<as well as braces so generic types in JSDoc do not break MDX builds. README, changelog, and broad CLI/generate/discovery tests cover the new behavior.Reviewed by Cursor Bugbot for commit 1da2bbd. Bugbot is set up for automated code reviews on this repo. Configure here.