[composable-controller] Fix BaseControllerV1Instance type and make ChildControllers a required type parameter
#4448
Assignees
Labels
Comments
Open
9 tasks
3 tasks
MajorLift
added a commit
that referenced
this issue
Aug 20, 2024
… safeguards (#4467) ## Overview This commit fixes issues with the `ComposableController` class's interface, and its logic for validating V1 and V2 controllers. These changes will enable `ComposableController` to function correctly downstream in the Mobile Engine, and eventually the Wallet Framework POC. ## Explanation The previous approach of generating mock controller classes from the `ComposableControllerState` input using the `GetChildControllers` was flawed, because the mock controllers would always be incomplete, complicating attempts at validation. Instead, we now rely on the downstream consumer to provide both a composed type of state schemas (`ComposableControllerState`) and a type union of the child controller instances (`ChildControllers`). For example, in mobile, we can use (with some adjustments) `EngineState` for the former, and `Controllers[keyof Controllers]` for the latter. The validation logic for V1 controllers has also been updated. Due to breaking changes made to the private properties of `BaseControllerV1` (#3959), mobile V1 controllers relying on versions prior to these changes were introduced were incompatible with the up-to-date `BaseControllerV1` version that the composable-controller package references. In this commit, the validator type `BaseControllerV1Instance` filters out the problematic private properties by using the `PublicInterface` type. Because the public API of `BaseControllerV1` has been relatively constant, this removes the type errors that previously occurred in mobile when passing V1 controllers into `ComposableController`. ## References - Closes #4448 - Contributes to #4213 - Next steps MetaMask/metamask-mobile#10073 - See MetaMask/metamask-mobile#10011 ## Changelog ### `@metamask/composable-controller` (major) ### Changed - **BREAKING:** Add two required generic parameters to the `ComposableController` class: `ComposedControllerState` (constrained by `LegacyComposableControllerStateConstraint`) and `ChildControllers` (constrained by `ControllerInstance`) ([#4467](#4467)). - **BREAKING:** The type guard `isBaseController` now validates that the input has an object-type property named `metadata` in addition to its existing checks. - **BREAKING:** The type guard `isBaseControllerV1` now validates that the input has object-type properties `config`, `state`, and function-type property `subscribe`, in addition to its existing checks. - **BREAKING:** Narrow `LegacyControllerStateConstraint` type from `BaseState | StateConstraint` to `BaseState & object | StateConstraint`. - Add an optional generic parameter `ControllerName` to the `RestrictedControllerMessengerConstraint` type, which extends `string` and defaults to `string`. ### Fixed - **BREAKING:** The `ComposableController` class raises a type error if a non-controller with no `state` property is passed into the `ChildControllers` generic parameter or the `controllers` constructor option. - Previously, a runtime error was thrown at class instantiation with no type-level enforcement. - When the `ComposableController` class is instantiated, its messenger now attempts to subscribe to all child controller `stateChange` events that are included in the messenger's events allowlist. - This was always the expected behavior, but a bug introduced in `@metamask/composable-controller@6.0.0` caused `stateChange` event subscriptions to fail. - `isBaseController` and `isBaseControllerV1` no longer return false negatives. - The `instanceof` operator is no longer used to validate that the input is a subclass of `BaseController` or `BaseControllerV1`. - The `ChildControllerStateChangeEvents` type checks that the child controller's state extends from the `StateConstraintV1` type instead of from `Record<string, unknown>`. ([#4467](#4467)) - V1 controllers define their state types using the `interface` keyword, which are incompatible with `Record<string, unknown>` by default. This resulted in `ChildControllerStateChangeEvents` failing to generate `stateChange` events for V1 controllers and returning `never`. ## 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 highlighted breaking changes using the "BREAKING" category above as appropriate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
References
@metamask/composable-controllerfrom^3.0.0to^6.0.2metamask-mobile#10011Motivation
The
ComposableControllerAPI is currently not working as expected in downstream consumers (mobile).Explanation
BaseControllerV1Instancetype needs to be fixed to accommodate controller versions used in mobile that inherit fromBaseControllerV1.ChildControllersoptional generic parameter should either be converted into a required parameter, or annotated in jsdoc to make it clear that a full list of controllers can be supplied by the consumer as a type argument.LegacyComposableControllerStateConstraintto{ [name: string]: Record<any, any> }, since index signature ofinterfacetypes used for V1 state objects is not fixed tostring.isBaseController()andisBaseControllerV1()are functioning as intended.The text was updated successfully, but these errors were encountered: