[base-controller] Fix any usage in BaseControllerV1
#3959
Merged
Conversation
This file contains 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
MajorLift
force-pushed
the
240222-base-controller-fix-any
branch
from
19671cf
to
c6e4651
Compare
MajorLift
force-pushed
the
240222-base-controller-fix-any
branch
from
c6e4651
to
0bd4386
Compare
MajorLift
changed the title
any usage in BaseControllerV1any usage in BaseControllerV1
mcmire
reviewed
Feb 23, 2024
| @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Changed | |||
|
|
|||
| - **BREAKING:** Change the types for `BaseControllerV1` class fields `initialConfig`, `initialState` from `C`, `S` to `Partial<C>`, `Partial<S>` ([#3959](https://github.com/MetaMask/core/pull/3959)) | |||
There was a problem hiding this comment.
It looks like these are private properties, so is this breaking?
There was a problem hiding this comment.
The constructor options config, state are handled slightly differently since there's no type assertion for the default parameter, but this probably doesn't warrant a changelog entry much less as a breaking change. Removed: 0a66987
| ### Changed | ||
|
|
||
| - **BREAKING:** Change the types for `BaseControllerV1` class fields `initialConfig`, `initialState` from `C`, `S` to `Partial<C>`, `Partial<S>` ([#3959](https://github.com/MetaMask/core/pull/3959)) | ||
| - **BREAKING:** Convert `BaseConfig`, `BaseState` interfaces to types ([#3959](https://github.com/MetaMask/core/pull/3959)) |
There was a problem hiding this comment.
Since we're aiming to remove BaseControllerV1, maybe we don't worry about converting these types? This way we can avoid a breaking change, at least for this PR.
There was a problem hiding this comment.
Reverted here: 8347a2a
…Replace with `as unknown as`
- `never` is assignable to all types
- Generic spread expressions for object literals are supported by TypeScript: microsoft/TypeScript#28234
… `initialState` from `C`, `S` to `Partial<C>`, `Partial<S>`
This reverts commit 93a997b.
MajorLift
force-pushed
the
240222-base-controller-fix-any
branch
from
6ab2e37
to
8347a2a
Compare
3 tasks
MajorLift
added a commit
that referenced
this pull request
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/[email protected]` 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
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.
Explanation
as unknown asinstead ofas any.BaseControllerV1class fieldsinitialConfig,initialStatefromC,StoPartial<C>,Partial<S>.C,Sobjects, sinceinternal{Config,State}will be populated withdefault{Config,State}.as never(neveris assignable to all types).References
base-controller: Replace use ofanywith proper types (non-test files only) #3715Changelog
@metamask/base-controllerChecklist