-
-
Notifications
You must be signed in to change notification settings - Fork 190
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
[composable-controller] Narrow class and messenger types by parameterizing over child controllers list #3952
Conversation
8fb745a
to
feb6c38
Compare
0c830ba
to
599fd44
Compare
ba71fbf
to
923d77d
Compare
bdc167c
to
0882483
Compare
716af2a
to
df1cdc8
Compare
6ef730e
to
0fa97c3
Compare
df1cdc8
to
b2b9059
Compare
717c66f
to
94019d1
Compare
0ab242a
to
0fa97c3
Compare
94019d1
to
3e9a5d2
Compare
0fa97c3
to
7d46b06
Compare
5f59bf9
to
58ba8bc
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.
Had some questions, but overall makes sense.
@@ -25,10 +25,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Changed | |||
|
|||
- **BREAKING:** Passing a non-controller into `controllers` constructor option now throws an error ([#3904](https://github.com/MetaMask/core/pull/3904)) | |||
- **BREAKING:** The `AllowedAction` parameter of the `ComposableControllerMessenger` type is narrowed from `string` to `never`, as `ComposableController` does not use any external controller actions ([#3904](https://github.com/MetaMask/core/pull/3904)) | |||
- **BREAKING:** The `AllowedActions` parameter of the `ComposableControllerMessenger` type is narrowed from `string` to `never`, as `ComposableController` does not use any external controller actions. ([#3904](https://github.com/MetaMask/core/pull/3904)) |
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 seems that we are updating the changelog for an older version, is this intentional? Are there changes we forgot to include for this version?
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.
Fixed to align with final changelog in PR description: 365aa02
P extends keyof ComposableControllerState = keyof ComposableControllerState, | ||
> = P extends string | ||
? ComposableControllerState[P] extends StateConstraint | ||
? { name: P; state: ComposableControllerState[P] } | ||
: BaseControllerV1< | ||
BaseConfig & Record<string, unknown>, | ||
BaseState & ComposableControllerState[P] |
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.
Nit: Can we use a non-one-letter name here for clarify? Perhaps ControllerName
?
P extends keyof ComposableControllerState = keyof ComposableControllerState, | |
> = P extends string | |
? ComposableControllerState[P] extends StateConstraint | |
? { name: P; state: ComposableControllerState[P] } | |
: BaseControllerV1< | |
BaseConfig & Record<string, unknown>, | |
BaseState & ComposableControllerState[P] | |
ControllerName extends keyof ComposableControllerState = keyof ComposableControllerState, | |
> = Key extends string | |
? ComposableControllerState[ControllerName] extends StateConstraint | |
? { name: ControllerName; state: ComposableControllerState[ControllerName] } | |
: BaseControllerV1< | |
BaseConfig & Record<string, unknown>, | |
BaseState & ComposableControllerState[ControllerName] |
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.
Fixed here: 25f2f0b
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.
Looks good!
Explanation
Currently, the allow lists of
ComposableControllerMessenger
are set tostring
, which is too permissive. EachComposableController
instance needs typing and allow lists that are specific to its set of input child controllers.To achieve this, the
ComposableController
class and its messenger are made polymorphic upon theControllerState
type, which defines the shape of the composed state.References
Changelog
@metamask/composable-controller
Added
RestrictedControllerMessengerConstraint
, which is the narrowest supertype of all controller-messenger instances.LegacyControllerStateConstraint
, a universal supertype for the controller state object, encompassing both BaseControllerV1 and BaseControllerV2 state.ComposableControllerStateConstraint
, the narrowest supertype for the composable controller state object.Changed
ComposableController
class is now a generic class that expects one generic argumentComposableControllerState
(#3952).ComposableController
class to be typed correctly, any of its child controllers that extendBaseControllerV1
must have an overriddenname
property that is defined using theas const
assertion.Checklist