feat(messenger): add delegateAll with exhaustive type checking#8338
Open
cryptodev-2s wants to merge 13 commits into
Open
feat(messenger): add delegateAll with exhaustive type checking#8338cryptodev-2s wants to merge 13 commits into
delegateAll with exhaustive type checking#8338cryptodev-2s wants to merge 13 commits into
Conversation
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. |
cryptodev-2s
force-pushed
the
add-messenger-delegate-all
branch
from
March 30, 2026 17:44
5bd4a95 to
f0e25c9
Compare
`delegate` accepts partial action/event lists without compile-time enforcement that all required items are present. `delegateAll` uses a branded intersection type to produce a clear TS error showing exactly which actions or events are missing.
cryptodev-2s
force-pushed
the
add-messenger-delegate-all
branch
from
April 7, 2026 16:59
f0e25c9 to
0bc2ac9
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. |
cryptodev-2s
marked this pull request as ready for review
July 24, 2026 13:39
cryptodev-2s
temporarily deployed
to
default-branch
July 24, 2026 13:39 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
mcmire
reviewed
Jul 24, 2026
mcmire
left a comment
mcmire
left a comment
Collaborator
There was a problem hiding this comment.
Overall strategy seems sound, but I had some suggestions.
| expect(childMessenger.call('Source:getValue')).toBe(42); | ||
| }); | ||
|
|
||
| it('produces a type error when an action is missing', () => { |
Collaborator
There was a problem hiding this comment.
I have found that Jest doesn't always check type errors, so I am not sure this will work as intended.
What are your thoughts about adding tstyche to this package and writing compile-time tests? We could include this test and also the one before it (since it also seems like that is meant to test the type and not the behavior).
Contributor
Author
There was a problem hiding this comment.
Good call. Added tstyche in e2fca54 and moved the exhaustiveness cases (missing action + own namespace exclusion) into Messenger.delegateAll.tst.ts. yarn test now runs unit tests and type tests.
Comment on lines
+1199
to
+1202
| const DelegatedActions extends readonly (MessengerActions<Delegatee> & | ||
| Action)['type'][], | ||
| const DelegatedEvents extends readonly (MessengerEvents<Delegatee> & | ||
| Event)['type'][], |
Collaborator
There was a problem hiding this comment.
Do we need const ... readonly here? If we take these away we can remove the type assertions below.
Suggested change
| const DelegatedActions extends readonly (MessengerActions<Delegatee> & | |
| Action)['type'][], | |
| const DelegatedEvents extends readonly (MessengerEvents<Delegatee> & | |
| Event)['type'][], | |
| DelegatedActions extends (MessengerActions<Delegatee> & Action)['type'][], | |
| DelegatedEvents extends (MessengerEvents<Delegatee> & Event)['type'][], |
Collaborator
There was a problem hiding this comment.
At the very least, we should extract the type first, then perform an intersection of them. We found in the past that performing an intersection first led to a "union type too complex to represent" error: #8748
Suggested change
| const DelegatedActions extends readonly (MessengerActions<Delegatee> & | |
| Action)['type'][], | |
| const DelegatedEvents extends readonly (MessengerEvents<Delegatee> & | |
| Event)['type'][], | |
| const DelegatedActions extends | |
| readonly (MessengerActions<Delegatee>['type'] & Action['type'])[], | |
| const DelegatedEvents extends readonly (MessengerEvents<Delegatee>['type'] & | |
| Event['type'])[], |
Or if we go with the above suggestion:
Suggested change
| const DelegatedActions extends readonly (MessengerActions<Delegatee> & | |
| Action)['type'][], | |
| const DelegatedEvents extends readonly (MessengerEvents<Delegatee> & | |
| Event)['type'][], | |
| DelegatedActions extends (MessengerActions<Delegatee>['type'] & | |
| Action['type'])[], | |
| DelegatedEvents extends (MessengerEvents<Delegatee>['type'] & | |
| Event['type'])[], |
Contributor
Author
There was a problem hiding this comment.
Done in 08fc1b3. Dropped const/readonly and switched to MessengerActions<Delegatee>['type'] & Action['type'] (same for events), matching delegate/revoke. Casts are gone as a result.
Comment on lines
+1224
to
+1228
| this.delegate({ | ||
| actions: actions as (MessengerActions<Delegatee> & Action)['type'][], | ||
| events: events as (MessengerEvents<Delegatee> & Event)['type'][], | ||
| messenger, | ||
| }); |
Collaborator
There was a problem hiding this comment.
Suggested change
| this.delegate({ | |
| actions: actions as (MessengerActions<Delegatee> & Action)['type'][], | |
| events: events as (MessengerEvents<Delegatee> & Event)['type'][], | |
| messenger, | |
| }); | |
| this.delegate({ actions, events, messenger }); |
Contributor
Author
There was a problem hiding this comment.
Done. Now this.delegate({ actions, events, messenger }).
| actions: RequireExhaustive< | ||
| NotNamespacedBy< | ||
| MessengerNamespace<Delegatee>, | ||
| (MessengerActions<Delegatee> & Action)['type'] |
Collaborator
There was a problem hiding this comment.
Similar optimization as above:
Suggested change
| (MessengerActions<Delegatee> & Action)['type'] | |
| MessengerActions<Delegatee>['type'] & Action['type'] |
Contributor
Author
There was a problem hiding this comment.
Done. Updated to MessengerActions<Delegatee>['type'] & Action['type'].
| events: RequireExhaustive< | ||
| NotNamespacedBy< | ||
| MessengerNamespace<Delegatee>, | ||
| (MessengerEvents<Delegatee> & Event)['type'] |
Collaborator
There was a problem hiding this comment.
Similar optimization as above:
Suggested change
| (MessengerEvents<Delegatee> & Event)['type'] | |
| MessengerEvents<Delegatee>['type'] & Event['type'] |
Contributor
Author
There was a problem hiding this comment.
Done. Updated to MessengerEvents<Delegatee>['type'] & Event['type'].
| Provided extends readonly string[], | ||
| > = [Exclude<Required, Provided[number]>] extends [never] | ||
| ? Provided | ||
| : Provided & { missingDelegations: Exclude<Required, Provided[number]> }; |
Collaborator
There was a problem hiding this comment.
Nit: Do we want to name this property something non-standard on purpose so engineers don't mistake it for a property they ought to set?
Suggested change
| : Provided & { missingDelegations: Exclude<Required, Provided[number]> }; | |
| // eslint-disable-next-line @typescript-eslint/naming-convention | |
| : Provided & { __MISSING_DELEGATIONS__: Exclude<Required, Provided[number]> }; |
Contributor
Author
There was a problem hiding this comment.
Good idea. Renamed to __MISSING_DELEGATIONS__ so it does not look like a real option.
Align delegateAll type params with delegate/revoke, drop casts, and rename the missing-delegations brand for clearer type errors.
Move exhaustiveness checks out of Jest into compile-time tests so missing delegations are verified by the type checker.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution MetaMask internal reviewing guidelines:
|
Keep Jest test files out of the build when excluding TSTyche files, format misc lint failures, and allow messenger's combined test script.
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 0f82791. Configure here.
Require every external delegatee action/event in the exhaustiveness check, even when the source type omits them, and cover missing events plus unsupported source deps in tstyche.
Mirror the Jest layout with one Messenger type test file and nested describes instead of a per-method file.
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
2 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
Messenger.delegate()accepts a partial list of actions/events without compile-time enforcement that all required items are present. This means missing a delegation (e.g., a newly addedAllowedAction) silently compiles and only fails at runtime.delegateAll()wrapsdelegate()with aRequireExhaustivebranded intersection type that produces a clear TypeScript error showing exactly which actions or events are missing from the list. Own-namespace actions of the delegatee are automatically excluded from the check.Example error when an action is missing:
References
Changelog
@metamask/messengerdelegateAll()method — strict alternative todelegate()that requires all external actions and events to be listedMessengerNamespace<M>utility type — extracts the namespace string literal from a Messenger typeRequireExhaustive<Required, Provided>utility type — validates tuple exhaustiveness at compile timeChecklist
Note
Low Risk
Additive API and type-level checks only;
delegateAllforwards to existingdelegatewith no runtime behavior change beyond new call sites.Overview
Adds
delegateAllonMessenger, a strict alternative todelegatethat must list every external action and event the child messenger needs (excluding the delegatee’s own namespace). Missing items fail at compile time via an internalRequireExhaustivebranded type that surfaces missing delegation keys; runtime behavior still delegates throughdelegate.Also exports
MessengerNamespaceto read a messenger’s namespace from its type, wires TSTyche compile-time tests (Messenger.tst.ts), splitstestintotest:unit+test:types, and excludes*.tst.tsfrom Jest coverage and the build.Reviewed by Cursor Bugbot for commit cb9e3aa. Bugbot is set up for automated code reviews on this repo. Configure here.