feat(messenger): add generate-action-types CLI tool as subpath export#8264
Open
cryptodev-2s wants to merge 29 commits intomainfrom
Open
feat(messenger): add generate-action-types CLI tool as subpath export#8264cryptodev-2s wants to merge 29 commits intomainfrom
generate-action-types CLI tool as subpath export#8264cryptodev-2s wants to merge 29 commits intomainfrom
Conversation
cryptodev-2s
changed the title
@metamask/messenger-generate-action-types packagegenerate-action-types CLI tool as subpath export
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Extract `scripts/generate-method-action-types.ts` into a publishable CLI package so it can be reused across repositories. - Scaffold `@metamask/messenger-generate-action-types` with bin entry - Split script into modular source files (parse, generate, check, fix) - Use named TypeScript imports, optional ESLint peer dependency - Update 44 consuming packages to use the new binary - Update package template, CODEOWNERS, teams.json, eslint config - Add ESLint config exception for Node.js module imports - Regenerate all action type files with updated header - Delete old `scripts/generate-method-action-types.ts`
The binary requires a build step before it can be used. Using tsx to run the source TypeScript directly avoids this dependency.
Move the CLI tool into @metamask/messenger as a subpath export instead of a separate package. This keeps the library lightweight while making the codegen available via optional peer dependencies. - Move source files to packages/messenger/src/generate-action-types/ - Add ./generate-action-types subpath export and bin entry - typescript, yargs, eslint are optional peerDependencies - @metamask/utils added as dependency (needed by codegen) - Remove standalone @metamask/messenger-generate-action-types package - Update 44 consuming packages to point to new location - Revert CODEOWNERS, teams.json, tsconfig, README changes
- Remove unused `MethodInfo.signature` field - Remove redundant `ControllerInfo.exposedMethods` (derivable from methods) - Extract shared ESLint types to `types.ts`, deduplicate across check/fix - Combine eslint/eslintStatic params into single `ESLint | null` object - Inline single-use `capitalize()` helper - Remove unused `Identifier` type import and `extractMethodSignature`
cryptodev-2s
force-pushed
the
feat/messenger-generate-action-types-package
branch
from
89da66f to
0202e79
Compare
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Mrtenz
requested changes
Mar 23, 2026
Recursively search subdirectories for source files, matching the snaps repo implementation. This allows packages with controllers or services in nested directories to be discovered automatically.
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
mcmire
reviewed
Mar 23, 2026
packages/messenger/package.json
Outdated
| "typedoc-plugin-missing-exports": "^2.0.0", | ||
| "typescript": "~5.3.3" | ||
| "typescript": "~5.3.3", | ||
| "yargs": "^17.7.2" |
Contributor
There was a problem hiding this comment.
Should these be in dependencies, since they're being referred to in code that we are publishing?
Member
There was a problem hiding this comment.
yargs probably, but TypeScript should probably be a peer dependency?
Contributor
There was a problem hiding this comment.
Now that you say it, yes, that would probably make more sense. I guess we want a peer dependency on ESLint for the same reason. They're tools that the project should already have, and we don't want to use a different version of them.
So 1) typescript and eslint should be in peer dependencies, but they should not be optional peer dependencies (we need them or else this script won't work) and 2) only yargs/@types/yargs needs to be in dependencies.
packages/messenger/src/generate-action-types/generate-content.ts
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Recursive search makes per-subdirectory scripts unnecessary in assets-controllers, notification-services-controller, and profile-sync-controller.
4 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 24, 2026
## Explanation The auto-generated method action type files previously referenced the generator script path in their header comment (`This file is auto generated by scripts/generate-method-action-types.ts`). This simplifies it to just `This file is auto generated.`. For context why we are making this change #8264 (comment) ## References N/A ## 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 - [ ] 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** > Only changes a comment string in generated `*-method-action-types.ts` files and the generator script; no runtime logic or types are modified beyond file header text. > > **Overview** > **Simplifies the header comment** across all auto-generated `*-method-action-types.ts` files by removing the explicit generator script path reference and leaving `This file is auto generated.` plus `Do not edit manually.` > > Updates `scripts/generate-method-action-types.ts` so future generations produce the new header format. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 472e809. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…te-action-types-package
No consumers import programmatically. The tool is CLI-only.
micaelae
pushed a commit
that referenced
this pull request
Mar 24, 2026
## Explanation The auto-generated method action type files previously referenced the generator script path in their header comment (`This file is auto generated by scripts/generate-method-action-types.ts`). This simplifies it to just `This file is auto generated.`. For context why we are making this change #8264 (comment) ## References N/A ## 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 - [ ] 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** > Only changes a comment string in generated `*-method-action-types.ts` files and the generator script; no runtime logic or types are modified beyond file header text. > > **Overview** > **Simplifies the header comment** across all auto-generated `*-method-action-types.ts` files by removing the explicit generator script path reference and leaving `This file is auto generated.` plus `Do not edit manually.` > > Updates `scripts/generate-method-action-types.ts` so future generations produce the new header format. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 472e809. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
- Refactor cli.ts: export main(args), parseCommandLineArguments(args), loadESLint() for testability; simplify auto-invocation guard - Add functional tests verifying exact generated output for controllers and services (file names and full type content) - Test variations: multiple methods with/without JSDoc, @param/@returns, controller+service in same dir, nested subdirectories, empty directory - Cover --check mode: up-to-date, out-of-date, and missing files - Remove coveragePathIgnorePatterns for cli.ts — now fully covered - 127 total tests, 100% coverage
Spawn the CLI as a subprocess via tsx + execa (same pattern as
create-release-branch). Tests verify exact generated output given
source files for controllers and services.
- File name conventions: {ClassName}-method-action-types.ts
- Full type content: action types, handler references, union types
- Variations: multiple methods with/without JSDoc, @param/@returns,
single-method service, controller+service in same dir, nested
subdirectories, empty dir
- --check mode: up-to-date, out-of-date, missing files
- Argument validation: exits 1 without --check or --fix
Per review feedback: - yargs: moved to dependencies (runtime dep of the CLI) - typescript, eslint, @metamask/utils: non-optional peerDependencies (required for the CLI to work, but provided by the consuming project) - Removed peerDependenciesMeta (no optional peers remaining)
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Required peer dependency failure silently swallowed
Medium Severity
loadESLint silently returns null when ESLint fails to load, even though eslint is a required (non-optional) peer dependency. The old script instantiated ESLint directly and would crash immediately on failure. Now, if ESLint has a broken installation or version mismatch, the CLI silently skips all formatting with no warning — producing unformatted output files in --fix mode and potentially false pass/fail results in --check mode.
mcmire
reviewed
Mar 25, 2026
Contributor
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Almost there! Left some more comments, mostly minor.
| if (hasErrors) { | ||
| console.error('\n💥 Some action type files are out of date or missing.'); | ||
| console.error( | ||
| 'Run `yarn generate-method-action-types --fix` to update them.', |
Contributor
There was a problem hiding this comment.
Does this command need to be updated?
| let tmpDir: string; | ||
|
|
||
| beforeEach(async () => { | ||
| tmpDir = await fs.promises.mkdtemp( |
Contributor
There was a problem hiding this comment.
Nit: @metamask/utils provides a createSandbox function if it's useful. Each test needs to be wrapped though: https://github.com/MetaMask/utils/blob/main/src/fs.ts#L243
| - Add `generate-action-types` CLI tool ([#8264](https://github.com/MetaMask/core/pull/8264)) | ||
| - Generates TypeScript action type files for controllers and services that define `MESSENGER_EXPOSED_METHODS`. | ||
| - Available as a CLI binary (`messenger-generate-action-types`). | ||
| - `@metamask/utils`, `typescript`, `yargs`, and `eslint` are optional peer dependencies, only required when using the codegen tool. |
Contributor
There was a problem hiding this comment.
Should this be updated to read:
Suggested change
| - `@metamask/utils`, `typescript`, `yargs`, and `eslint` are optional peer dependencies, only required when using the codegen tool. | |
| - `typescript` and `eslint` are peer dependencies. |
| "typescript": "~5.3.3" | ||
| }, | ||
| "peerDependencies": { | ||
| "@metamask/utils": "^11.9.0", |
Contributor
There was a problem hiding this comment.
Should this be a regular dependency as well? I think we can allow this to be whatever version works for this use case, we don't need to require that the consuming project uses this exact version.
| const errors = eslint.getErrorResults(results); | ||
| if (errors.length > 0) { | ||
| console.error('❌ ESLint errors:', errors); | ||
| globalThis.process.exitCode = 1; |
Contributor
There was a problem hiding this comment.
Do we need to use globalThis. here? process is already a global in Node.
Suggested change
| globalThis.process.exitCode = 1; | |
| process.exitCode = 1; |
Contributor
There was a problem hiding this comment.
💡 Having seen the @metamask/messenger-docs package I am now curious whether there is some package can we use that would allow us to simplify this file (particularly extracting the JSDoc). It's out of scope for this PR but we might want to think about this.
| const originalExitCode = globalThis.process.exitCode; | ||
|
|
||
| beforeEach(async () => { | ||
| tmpDir = await fs.promises.mkdtemp( |
Contributor
There was a problem hiding this comment.
Same as cli.test.ts — we might consider using createSandbox from @metamask/utils.
|
|
||
| afterEach(async () => { | ||
| await fs.promises.rm(tmpDir, { recursive: true, force: true }); | ||
| globalThis.process.exitCode = originalExitCode; |
Contributor
There was a problem hiding this comment.
Nit: Instead of allowing this file to set the exit code, you might consider only allowing cli.ts to do this. That way you don't need to save and restore the exit code here. (The exit code is global to the entire process so it seems worth it to limit its scope.)
| @@ -0,0 +1,115 @@ | |||
| import * as fs from 'node:fs'; | |||
Contributor
There was a problem hiding this comment.
Nit: Is it necessary to use node: qualifiers when importing built-in modules? We don't really do this elsewhere so it would be inconsistent with our existing code style.
| }); | ||
| return { | ||
| instance, | ||
| outputFixes: ESLintClass.outputFixes.bind(ESLintClass), |
Contributor
There was a problem hiding this comment.
What is the purpose of wrapping these methods? I'm curious if it would be better to simply return the ESLint instance from this function, and then where we want to call outputFixes we use ESLint.outputFixes. As it is, by bundling outputFixes and getErrorResults with the ESLint instance it makes it seem like these are class (instance) methods, when in fact they are static methods.
…te-action-types-package
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| "@metamask/utils": "^11.9.0", | ||
| "eslint": ">=8", | ||
| "typescript": "~5.3.3" | ||
| }, |
There was a problem hiding this comment.
Peer dependencies not marked optional despite documentation claiming so
Medium Severity
The peerDependencies for @metamask/utils, eslint, and typescript are declared as required, but the CHANGELOG states they are "optional peer dependencies, only required when using the codegen tool." Without a peerDependenciesMeta section marking each as optional: true, npm/yarn will emit missing-peer-dependency warnings for every consumer of @metamask/messenger — even those that only use the core Messenger class and never invoke the CLI.
Additional Locations (1)
| }, | ||
| "dependencies": { | ||
| "yargs": "^17.7.2" | ||
| }, |
There was a problem hiding this comment.
Runtime yargs dependency shipped to all library consumers
Medium Severity
yargs is listed as a production dependency of @metamask/messenger, meaning every consumer of the package — including those only importing the core Messenger class — will install yargs and its transitive dependencies. This contradicts the PR goal that "the core Messenger library remains lightweight," since yargs is only needed by the CLI codegen tool.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
The script
scripts/generate-method-action-types.tsgenerates TypeScript action type files for controllers/services that defineMESSENGER_EXPOSED_METHODS. It was a monorepo-local script invoked viatsx ../../scripts/generate-method-action-types.tsfrom 44 packages.This PR moves it into
@metamask/messengeras a CLI tool, keeping the library lightweight while making the codegen reusable in other repositories.What changed:
generate-action-types/subdirectory underpackages/messenger/src/with modular source files (parse-source.ts,generate-content.ts,check.ts,fix.ts,cli.ts)messenger-generate-action-typesbin entry to@metamask/messengeryargsis the only runtime dependency;@metamask/utils,typescript, andeslintare non-optional peer dependencies (required for the CLI, but provided by the consuming project). The coreMessengerlibrary remains lightweight.tsx ../../packages/messenger/src/generate-action-types/cli.tsscripts/generate-method-action-types.tsexeca+tsxverify exact generated output for controllers and servicesWorking examples in external repos:
References
Checklist
Note
Medium Risk
Moderate risk because it changes the codegen entrypoint across many packages and introduces optional ESLint-driven formatting, which could affect generated file output and CI checks if environments differ.
Overview
Adds a new
@metamask/messengercodegen CLI (messenger-generate-action-types) that discovers controllers/services withMESSENGER_EXPOSED_METHODS, generates*-method-action-types.tsfiles, and supports--fix/--checkwith optional ESLint formatting.Migrates the repo to the new tool by deleting
scripts/generate-method-action-types.ts, updating ~44 packages’generate-method-action-typesscripts to invoke the newpackages/messenger/src/generate-action-types/cli.ts, and adjusting ESLint config to allow Node built-ins in the new generator sources. A few generated action-type files change slightly (header/template string usage and JSDoc wording) to match the new generator output.Written by Cursor Bugbot for commit f24ab34. This will update automatically on new commits. Configure here.