-
-
Notifications
You must be signed in to change notification settings - Fork 250
feat: Migrate next
BaseController class to new Messenger package
#6318
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
Conversation
2b24ae4
to
52e6756
Compare
5f65d0e
to
afa4dc5
Compare
6d9f187
to
513bb4b
Compare
/** | ||
* The controller messenger. This is used to interact with other parts of the application. | ||
*/ | ||
protected messagingSystem: ControllerMessenger; |
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 agreed recently in Slack to rename this property, but this will be done in a later PR.
* This is the same as the `messagingSystem` property, but has a type that only lets us use | ||
* actions and events that are part of the `BaseController` class. | ||
*/ | ||
readonly #messenger: 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.
I added a duplicate reference to the messenger here as a private instance variable just to avoid the need to cast it each time it's internally referenced.
} | ||
} | ||
|
||
it('should allow messaging between controllers', () => { |
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.
This test has the best complete example so far
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.
LGTM! Great to finally see this delegation in action, at least in the tests 🚀
next
BaseController class to new Messenger packagenext
BaseController class to new Messenger package
513bb4b
to
ae71891
Compare
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.
LGTM!
ae71891
to
67c7853
Compare
Explanation
Migrate the upcoming
BaseController
class to use the new@metamask/messenger
package. This includes replacingRestrictedMessenger
with the newMessenger
class that supports delegation.References
Relates to #5626
Checklist