-
-
Notifications
You must be signed in to change notification settings - Fork 295
feat(messenger): add delegateAll with exhaustive type checking
#8338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
cryptodev-2s
wants to merge
15
commits into
main
Choose a base branch
from
add-messenger-delegate-all
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
0ae874d
feat(messenger): add `delegateAll` with exhaustive type checking
cryptodev-2s cc5b501
docs: add changelog entry for `delegateAll`
cryptodev-2s 5517720
fix: address lint errors in delegateAll
cryptodev-2s e17d99a
fix: use @ts-expect-error for missing action type test
cryptodev-2s 0bc2ac9
fix: add assertion to satisfy jest/expect-expect
cryptodev-2s fcbfad9
Merge branch 'main' into add-messenger-delegate-all
cryptodev-2s 6ca6fbc
test: silence :stateChange lint in messenger
cryptodev-2s 08fc1b3
fix(messenger): address delegateAll review feedback
cryptodev-2s e2fca54
test(messenger): add tstyche coverage for delegateAll
cryptodev-2s a3bb887
fix(messenger): restore test excludes and satisfy constraints
cryptodev-2s 0f82791
Merge branch 'main' into add-messenger-delegate-all
cryptodev-2s 7c68fdf
fix(messenger): fail loudly when source lacks required delegations
cryptodev-2s cb9e3aa
test(messenger): consolidate type tests into Messenger.tst.ts
cryptodev-2s 6af8462
Merge branch 'main' into add-messenger-delegate-all
cryptodev-2s 22ed38b
fix(messenger): exclude TSTyche tests from lint:tsc
cryptodev-2s File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| import { describe, expect, test } from 'tstyche'; | ||
|
|
||
| import { Messenger } from './Messenger.js'; | ||
|
|
||
| describe('Messenger', () => { | ||
| describe('delegateAll', () => { | ||
| type ActionA = { type: 'A:getValue'; handler: () => number }; | ||
| type ActionB = { type: 'B:getName'; handler: () => string }; | ||
| type ChildOwnAction = { type: 'Child:doStuff'; handler: () => void }; | ||
| type EventA = { | ||
| type: 'A:stateChange'; | ||
| payload: [{ value: number }]; | ||
| }; | ||
| type EventB = { | ||
| type: 'B:nameChange'; | ||
| payload: [{ name: string }]; | ||
| }; | ||
|
|
||
| test('accepts a complete list of external actions and events', () => { | ||
| const source = new Messenger< | ||
| 'Source', | ||
| ActionA | ActionB | ChildOwnAction, | ||
| EventA | EventB | ||
| >({ namespace: 'Source' }); | ||
| const child = new Messenger< | ||
| 'Child', | ||
| ActionA | ActionB | ChildOwnAction, | ||
| EventA | EventB | ||
| >({ namespace: 'Child' }); | ||
|
|
||
| expect( | ||
| source.delegateAll({ | ||
| messenger: child, | ||
| actions: ['A:getValue', 'B:getName'], | ||
| events: ['A:stateChange', 'B:nameChange'], | ||
| }), | ||
| ).type.not.toRaiseError(); | ||
| }); | ||
|
|
||
| test('excludes the delegatee own-namespace actions from the exhaustiveness check', () => { | ||
| const source = new Messenger<'Source', ActionA | ChildOwnAction, never>({ | ||
| namespace: 'Source', | ||
| }); | ||
| const child = new Messenger<'Child', ActionA | ChildOwnAction, never>({ | ||
| namespace: 'Child', | ||
| }); | ||
|
|
||
| expect( | ||
| source.delegateAll({ | ||
| messenger: child, | ||
| actions: ['A:getValue'], | ||
| events: [], | ||
| }), | ||
| ).type.not.toRaiseError(); | ||
| }); | ||
|
|
||
| test('raises a type error when an external action is missing', () => { | ||
| const source = new Messenger<'Source', ActionA | ActionB, never>({ | ||
| namespace: 'Source', | ||
| }); | ||
| const child = new Messenger<'Child', ActionA | ActionB, never>({ | ||
| namespace: 'Child', | ||
| }); | ||
|
|
||
| expect( | ||
| source.delegateAll({ | ||
| messenger: child, | ||
| actions: ['A:getValue'], | ||
| events: [], | ||
| }), | ||
| ).type.toRaiseError(); | ||
| }); | ||
|
|
||
| test('raises a type error when an external event is missing', () => { | ||
| const source = new Messenger<'Source', never, EventA | EventB>({ | ||
| namespace: 'Source', | ||
| }); | ||
| const child = new Messenger<'Child', never, EventA | EventB>({ | ||
| namespace: 'Child', | ||
| }); | ||
|
|
||
| expect( | ||
| source.delegateAll({ | ||
| messenger: child, | ||
| actions: [], | ||
| events: ['A:stateChange'], | ||
| }), | ||
| ).type.toRaiseError(); | ||
| }); | ||
|
|
||
| test('raises a type error when the source cannot provide a required external action', () => { | ||
| const source = new Messenger<'Source', ActionA, never>({ | ||
| namespace: 'Source', | ||
| }); | ||
| const child = new Messenger<'Child', ActionA | ActionB, never>({ | ||
| namespace: 'Child', | ||
| }); | ||
|
|
||
| expect( | ||
| source.delegateAll({ | ||
| messenger: child, | ||
| actions: ['A:getValue'], | ||
| events: [], | ||
| }), | ||
| ).type.toRaiseError(); | ||
| }); | ||
| }); | ||
| }); |
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you moved all of the tests to
Messenger.tst.ts. Maybe we should still have one test which verifies that the behavior ofdelegateAllis as we expect (i.e., it delegates the requested actions and events to the given messenger)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have those in Messenger.test.ts under describe('delegateAll') https://github.com/MetaMask/core/pull/8338/changes#diff-7c70b8b38fb189e7496767fa9487bf0bbafe014e2fa5f9efb3224fda03fdea27R1934: one for actions + events, and one for actions with an empty events list. Only the compile-time cases moved to Messenger.tst.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, you're right. Thanks.