-
-
Notifications
You must be signed in to change notification settings - Fork 183
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] Subscribe to stateChange
events of V1 controllers with messenger, type fixes
#3964
[composable-controller] Subscribe to stateChange
events of V1 controllers with messenger, type fixes
#3964
Changes from all commits
4436c4a
4f29a28
4f1db0c
31b7e28
a0bdc22
3bd6144
cb68d6c
ae8aaf5
f98a735
7d46b06
811a94a
e93ea79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,51 +1,57 @@ | ||
import { BaseController, BaseControllerV1 } from '@metamask/base-controller'; | ||
import type { | ||
RestrictedControllerMessenger, | ||
BaseState, | ||
ActionConstraint, | ||
BaseConfig, | ||
StateMetadata, | ||
BaseState, | ||
EventConstraint, | ||
RestrictedControllerMessenger, | ||
StateConstraint, | ||
} from '@metamask/base-controller'; | ||
import { isValidJson } from '@metamask/utils'; | ||
import type { Patch } from 'immer'; | ||
|
||
export const controllerName = 'ComposableController'; | ||
|
||
// TODO: Remove this type once `BaseControllerV2` migrations are completed for all controllers. | ||
/** | ||
* A type encompassing all controller instances that extend from `BaseControllerV1`. | ||
* A universal subtype of all controller instances that extend from `BaseControllerV1`. | ||
* Any `BaseControllerV1` instance can be assigned to this type. | ||
* | ||
* Note that this type is not the greatest subtype or narrowest supertype of all `BaseControllerV1` instances. | ||
* This type is therefore unsuitable for general use as a type constraint, and is only intended for use within the ComposableController. | ||
*/ | ||
export type BaseControllerV1Instance = | ||
// `any` is used to include all `BaseControllerV1` instances. | ||
// `any` is used so that all `BaseControllerV1` instances are assignable to this type. | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
BaseControllerV1<any, any>; | ||
|
||
/** | ||
* A type encompassing all controller instances that extend from `BaseController` (formerly `BaseControllerV2`). | ||
* A universal subtype of all controller instances that extend from `BaseController` (formerly `BaseControllerV2`). | ||
* Any `BaseController` instance can be assigned to this type. | ||
* | ||
* The `BaseController` class itself can't be used directly as a type representing all of its subclasses, | ||
* because the generic parameters it expects require knowing the exact shape of the controller's state and messenger. | ||
* Note that this type is not the greatest subtype or narrowest supertype of all `BaseController` instances. | ||
* This type is therefore unsuitable for general use as a type constraint, and is only intended for use within the ComposableController. | ||
* | ||
* Instead, we look for an object with the `BaseController` properties that we use in the ComposableController (name and state). | ||
* For this reason, we only look for `BaseController` properties that we use in the ComposableController (name and state). | ||
*/ | ||
export type BaseControllerV2Instance = { | ||
export type BaseControllerInstance = { | ||
name: string; | ||
state: StateConstraint; | ||
}; | ||
|
||
// TODO: Remove `BaseControllerV1Instance` member once `BaseControllerV2` migrations are completed for all controllers. | ||
/** | ||
* A type encompassing all controller instances that extend from `BaseControllerV1` or `BaseController`. | ||
* A universal subtype of all controller instances that extend from `BaseController` (formerly `BaseControllerV2`) or `BaseControllerV1`. | ||
* Any `BaseController` or `BaseControllerV1` instance can be assigned to this type. | ||
* | ||
* Note that this type is not the greatest subtype or narrowest supertype of all `BaseController` and `BaseControllerV1` instances. | ||
* This type is therefore unsuitable for general use as a type constraint, and is only intended for use within the ComposableController. | ||
*/ | ||
export type ControllerInstance = | ||
| BaseControllerV1Instance | ||
| BaseControllerV2Instance; | ||
| BaseControllerInstance; | ||
|
||
/** | ||
* Determines if the given controller is an instance of BaseControllerV1 | ||
* Determines if the given controller is an instance of `BaseControllerV1` | ||
* @param controller - Controller instance to check | ||
* @returns True if the controller is an instance of BaseControllerV1 | ||
* TODO: Deprecate once `BaseControllerV2` migrations are completed for all controllers. | ||
* @returns True if the controller is an instance of `BaseControllerV1` | ||
*/ | ||
export function isBaseControllerV1( | ||
controller: ControllerInstance, | ||
|
@@ -67,13 +73,23 @@ export function isBaseControllerV1( | |
} | ||
|
||
/** | ||
* Determines if the given controller is an instance of BaseController | ||
* Determines if the given controller is an instance of `BaseController` | ||
* @param controller - Controller instance to check | ||
* @returns True if the controller is an instance of BaseController | ||
* @returns True if the controller is an instance of `BaseController` | ||
*/ | ||
export function isBaseController( | ||
controller: ControllerInstance, | ||
): controller is BaseController<never, never, never> { | ||
): controller is BaseController< | ||
string, | ||
StateConstraint, | ||
RestrictedControllerMessenger< | ||
string, | ||
ActionConstraint, | ||
EventConstraint, | ||
string, | ||
string | ||
> | ||
> { | ||
return ( | ||
'name' in controller && | ||
typeof controller.name === 'string' && | ||
|
@@ -83,10 +99,10 @@ export function isBaseController( | |
); | ||
} | ||
|
||
// TODO: Replace `any` with `Json` once `BaseControllerV2` migrations are completed for all controllers. | ||
export type ComposableControllerState = { | ||
// `any` is used here to disable the `BaseController` type constraint which expects state properties to extend `Record<string, Json>`. | ||
// `ComposableController` state needs to accommodate `BaseControllerV1` state objects that may have properties wider than `Json`. | ||
// TODO: Replace `any` with `Json` once `BaseControllerV2` migrations are completed for all controllers. | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
[name: string]: Record<string, any>; | ||
}; | ||
|
@@ -142,7 +158,7 @@ export class ComposableController extends BaseController< | |
|
||
super({ | ||
name: controllerName, | ||
metadata: controllers.reduce<StateMetadata<ComposableControllerState>>( | ||
metadata: controllers.reduce( | ||
(metadata, controller) => ({ | ||
...metadata, | ||
[controller.name]: isBaseController(controller) | ||
|
@@ -151,12 +167,9 @@ export class ComposableController extends BaseController< | |
}), | ||
{}, | ||
), | ||
state: controllers.reduce<ComposableControllerState>( | ||
(state, controller) => { | ||
return { ...state, [controller.name]: controller.state }; | ||
}, | ||
{}, | ||
), | ||
state: controllers.reduce((state, controller) => { | ||
return { ...state, [controller.name]: controller.state }; | ||
}, {}), | ||
messenger, | ||
}); | ||
|
||
|
@@ -168,33 +181,33 @@ export class ComposableController extends BaseController< | |
/** | ||
* Constructor helper that subscribes to child controller state changes. | ||
* @param controller - Controller instance to update | ||
* TODO: Remove `isBaseControllerV1` branch once `BaseControllerV2` migrations are completed for all controllers. | ||
*/ | ||
#updateChildController(controller: ControllerInstance): void { | ||
if (!isBaseController(controller) && !isBaseControllerV1(controller)) { | ||
throw new Error( | ||
'Invalid controller: controller must extend from BaseController or BaseControllerV1', | ||
); | ||
} | ||
|
||
const { name } = controller; | ||
if (isBaseControllerV1(controller)) { | ||
controller.subscribe((childState) => { | ||
this.update((state) => ({ | ||
...state, | ||
[name]: childState, | ||
})); | ||
}); | ||
} else if (isBaseController(controller)) { | ||
if ( | ||
(isBaseControllerV1(controller) && 'messagingSystem' in controller) || | ||
isBaseController(controller) | ||
) { | ||
this.messagingSystem.subscribe( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a BaseControllerV1 controller has a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point! Fixed here: 811a94a |
||
`${name}:stateChange`, | ||
(childState: StateConstraint) => { | ||
if (isValidJson(childState)) { | ||
this.update((state) => ({ | ||
...state, | ||
[name]: childState, | ||
})); | ||
} | ||
(childState: Record<string, unknown>) => { | ||
this.update((state) => { | ||
Object.assign(state, { [name]: childState }); | ||
}); | ||
}, | ||
); | ||
} else { | ||
throw new Error( | ||
'Invalid controller: controller must extend from BaseController or BaseControllerV1', | ||
); | ||
} else if (isBaseControllerV1(controller)) { | ||
controller.subscribe((childState) => { | ||
this.update((state) => { | ||
Object.assign(state, { [name]: childState }); | ||
}); | ||
}); | ||
} | ||
} | ||
} | ||
|
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.
👍🏻