-
-
Notifications
You must be signed in to change notification settings - Fork 252
feat: Migrate controller guidelines and examples to new Messenger #6335
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
base: main
Are you sure you want to change the base?
Conversation
815bf50
to
39756ca
Compare
ae71891
to
67c7853
Compare
0a1a16b
to
0bdc3d1
Compare
0bdc3d1
to
7bbe29c
Compare
Tests that used the `@metamask/base-controller/next` export were not working correctly until after a build was completed. They were using the built code rather than the source code. On CI the tests run without any build, so they would always fail. This was resolved up updating the TSConfig and Jest config to map the `next` export to the correct source code. This unblocks #6335 as well as other controller migrations to the upcoming BaseController version.
Tests that used the `@metamask/base-controller/next` export were not working correctly until after a build was completed. They were using the built code rather than the source code. On CI the tests run without any build, so they would always fail. This was resolved up updating the TSConfig and Jest config to map the `next` export to the correct source code. This unblocks #6335 as well as other controller migrations to the upcoming BaseController version.
This comment was marked as resolved.
This comment was marked as resolved.
Tests that used the `@metamask/base-controller/next` export were not working correctly until after a build was completed. They were using the built code rather than the source code. On CI the tests run without any build, so they would always fail. This was resolved up updating the TSConfig and Jest config to map the `next` export to the correct source code. This unblocks #6335 as well as other controller migrations to the upcoming BaseController version.
Tests that used the `@metamask/base-controller/next` export were not working correctly until after a build was completed. They were using the built code rather than the source code. On CI the tests run without any build, so they would always fail. This was resolved up updating the TSConfig and Jest config to map the `next` export to the correct source code. This unblocks #6335 as well as other controller migrations to the upcoming BaseController version.
…6349) ## Explanation Tests that used the `@metamask/base-controller/next` export were not working correctly until after a build was completed. They were using the built code rather than the source code. On CI the tests run without any build, so they would always fail. This was resolved up updating the TSConfig and Jest config to map the `next` export to the correct source code. This unblocks #6335 as well as other controller migrations to the upcoming BaseController version. ## References Relates to #5626 ## 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/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
c4836ce
to
d669a35
Compare
Rebased to resolve conflict (the messenger package publish) |
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.
Had some minor comments, but overall these changes make sense.
packages/sample-controllers/src/sample-gas-prices-controller.test.ts
Outdated
Show resolved
Hide resolved
0f4215d
to
5d451a9
Compare
e8652f9
to
e59a0bb
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!
type RootMessenger = Messenger< | ||
ExtractAvailableAction<SampleGasPricesControllerMessenger>, | ||
ExtractAvailableEvent<SampleGasPricesControllerMessenger> | ||
// Use `string` rather than 'Root' here to allow registering actions and publishing events from |
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.
Oh that's a good idea, I hadn't thought of that.
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.
It would have been a good idea if it worked 😅 I forgot there are runtime checks for this, tests are failing. Looking into it now.
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.
Alright, it works now.
## Explanation Fix mistakes and improve consistency and formatting in comment examples in the `@metamask/sample-controllers` package. ## References Extracted from #6335 ## 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/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
ed42ac9
to
f2bb7e9
Compare
f2bb7e9
to
0f31c4e
Compare
Migrate the controller guidelines and the `sample-controllers` package to the `next` export of the `@metamask/base-controller` package and the new `Messenger` class from `@metamask/messenger`.
Co-authored-by: Elliot Winkler <[email protected]>
0f31c4e
to
013b836
Compare
Explanation
Migrate the controller guidelines and the
sample-controllers
package to thenext
export of the@metamask/base-controller
package and the newMessenger
class from@metamask/messenger
.References
Relates to #5626
Checklist
Note
Migrates controller guidelines and sample controllers to the new
@metamask/messenger
Messenger
and@metamask/base-controller/next
, updating APIs, tests, and docs, and adding the messenger dependency.docs/controller-guidelines.md
anddocs/data-services.md
to useMessenger
(namespace/parent,delegate
,subscribe
/publish
) instead ofRestrictedMessenger
and "messaging system".*Actions
and*Events
directly toMessenger
; clarify external unions and non-export rules.messagingSystem
withmessenger
API; addincludeInDebugSnapshot
metadata key.SampleGasPricesController
andSamplePetnamesController
to@metamask/base-controller/next
andMessenger
; replaceRestrictedMessenger
types andmessagingSystem.*
calls withmessenger.*
.SampleGasPricesService
) and tests to newMessenger
(MockAnyNamespace
,MessengerActions
/MessengerEvents
) and root/child messenger delegation.@metamask/messenger
dependency and TS project references; updateyarn.lock
.Messenger
.Written by Cursor Bugbot for commit 013b836. This will update automatically on new commits. Configure here.