diff --git a/packages/composable-controller/CHANGELOG.md b/packages/composable-controller/CHANGELOG.md index 26688cf855f..32709313aef 100644 --- a/packages/composable-controller/CHANGELOG.md +++ b/packages/composable-controller/CHANGELOG.md @@ -7,6 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add and export functions `isBaseControllerV1` and `isBaseController`, which are type guards for validating controller instances ([#3904](https://github.com/MetaMask/core/pull/3904)) +- Add and export types `BaseControllerV1Instance`, `BaseControllerV2Instance`, `ControllerInstance` which are the narrowest supertypes for all controllers extending from, respectively, `BaseControllerV1`, `BaseController`, and both ([#3904](https://github.com/MetaMask/core/pull/3904)) + +### Changed + +- **BREAKING:** Passing a non-controller into `controllers` constructor option now throws an error ([#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)) +- Add `@metamask/utils` ^8.3.0 as a dependency. ([#3904](https://github.com/MetaMask/core/pull/3904)) + +### Removed + +- **BREAKING:** Remove `ControllerList` as an exported type. ([#3904](https://github.com/MetaMask/core/pull/3904)) + ## [5.0.1] ### Changed diff --git a/packages/composable-controller/package.json b/packages/composable-controller/package.json index 432b6b3c03f..53ad6a93be6 100644 --- a/packages/composable-controller/package.json +++ b/packages/composable-controller/package.json @@ -31,10 +31,12 @@ "test:watch": "jest --watch" }, "dependencies": { - "@metamask/base-controller": "^4.1.1" + "@metamask/base-controller": "^4.1.1", + "@metamask/utils": "^8.3.0" }, "devDependencies": { "@metamask/auto-changelog": "^3.4.4", + "@metamask/json-rpc-engine": "^7.3.2", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", "immer": "^9.0.6", diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index 4e5d944fa1f..868605deb1e 100644 --- a/packages/composable-controller/src/ComposableController.test.ts +++ b/packages/composable-controller/src/ComposableController.test.ts @@ -7,10 +7,11 @@ import { BaseControllerV1, ControllerMessenger, } from '@metamask/base-controller'; +import { JsonRpcEngine } from '@metamask/json-rpc-engine'; import type { Patch } from 'immer'; import * as sinon from 'sinon'; -import type { ComposableControllerStateChangeEvent } from './ComposableController'; +import type { ComposableControllerEvents } from './ComposableController'; import { ComposableController } from './ComposableController'; // Mock BaseController classes @@ -106,7 +107,10 @@ describe('ComposableController', () => { describe('BaseControllerV1', () => { it('should compose controller state', () => { - const composableMessenger = new ControllerMessenger().getRestricted({ + const composableMessenger = new ControllerMessenger< + never, + ComposableControllerEvents + >().getRestricted({ name: 'ComposableController', }); const controller = new ComposableController({ @@ -123,7 +127,7 @@ describe('ComposableController', () => { it('should notify listeners of nested state change', () => { const controllerMessenger = new ControllerMessenger< never, - ComposableControllerStateChangeEvent + ComposableControllerEvents >(); const composableMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', @@ -176,7 +180,7 @@ describe('ComposableController', () => { it('should notify listeners of nested state change', () => { const controllerMessenger = new ControllerMessenger< never, - ComposableControllerStateChangeEvent | FooControllerEvent + ComposableControllerEvents | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -240,7 +244,7 @@ describe('ComposableController', () => { const barController = new BarController(); const controllerMessenger = new ControllerMessenger< never, - ComposableControllerStateChangeEvent | FooControllerEvent + ComposableControllerEvents | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -280,7 +284,7 @@ describe('ComposableController', () => { const barController = new BarController(); const controllerMessenger = new ControllerMessenger< never, - ComposableControllerStateChangeEvent | FooControllerEvent + ComposableControllerEvents | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -335,5 +339,35 @@ describe('ComposableController', () => { }), ).toThrow('Messaging system is required'); }); + + it('should throw if composing a controller that does not extend from BaseController', () => { + const notController = new JsonRpcEngine(); + const controllerMessenger = new ControllerMessenger< + never, + FooControllerEvent + >(); + const fooControllerMessenger = controllerMessenger.getRestricted< + 'FooController', + never, + never + >({ + name: 'FooController', + }); + const fooController = new FooController(fooControllerMessenger); + const composableControllerMessenger = controllerMessenger.getRestricted({ + name: 'ComposableController', + allowedEvents: ['FooController:stateChange'], + }); + expect( + () => + new ComposableController({ + // @ts-expect-error - Suppressing type error to test for runtime error handling + controllers: [notController, fooController], + messenger: composableControllerMessenger, + }), + ).toThrow( + 'Invalid controller: controller must extend from BaseController or BaseControllerV1', + ); + }); }); }); diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index 6d4f6eb7d83..fc3fd2565be 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -2,57 +2,114 @@ import { BaseController, BaseControllerV1 } from '@metamask/base-controller'; import type { ControllerStateChangeEvent, RestrictedControllerMessenger, + BaseState, + BaseConfig, + StateMetadata, } from '@metamask/base-controller'; +import { isValidJson, type Json } from '@metamask/utils'; export const controllerName = 'ComposableController'; -/* - * This type encompasses controllers based on either BaseControllerV1 or - * BaseController. The BaseController type can't be included directly - * because the generic parameters it expects require knowing the exact state - * shape, so instead we look for an object with the BaseController properties - * that we use in the ComposableController (name and state). +// TODO: Remove this type once `BaseControllerV2` migrations are completed for all controllers. +/** + * A type encompassing all controller instances that extend from `BaseControllerV1`. */ -type ControllerInstance = - // TODO: Replace `any` with type +export type BaseControllerV1Instance = + // `any` is used to include all `BaseControllerV1` instances. // eslint-disable-next-line @typescript-eslint/no-explicit-any - BaseControllerV1 | { name: string; state: Record }; + BaseControllerV1; /** - * List of child controller instances + * A type encompassing all controller instances that extend from `BaseController` (formerly `BaseControllerV2`). + * + * 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. + * + * Instead, we look for an object with the `BaseController` properties that we use in the ComposableController (name and state). */ -export type ControllerList = ControllerInstance[]; +export type BaseControllerV2Instance = { + name: string; + state: Record; +}; + +// TODO: Remove `BaseControllerV1Instance` member once `BaseControllerV2` migrations are completed for all controllers. +/** + * A type encompassing all controller instances that extend from `BaseControllerV1` or `BaseController`. + */ +export type ControllerInstance = + | BaseControllerV1Instance + | BaseControllerV2Instance; /** * 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. */ -function isBaseControllerV1( +export function isBaseControllerV1( controller: ControllerInstance, - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any -): controller is BaseControllerV1 { - return controller instanceof BaseControllerV1; +): controller is BaseControllerV1< + BaseConfig & Record, + BaseState & Record +> { + return ( + 'name' in controller && + typeof controller.name === 'string' && + 'defaultConfig' in controller && + typeof controller.defaultConfig === 'object' && + 'defaultState' in controller && + typeof controller.defaultState === 'object' && + 'disabled' in controller && + typeof controller.disabled === 'boolean' && + controller instanceof BaseControllerV1 + ); +} + +/** + * 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 + */ +export function isBaseController( + controller: ControllerInstance, +): controller is BaseController { + return ( + 'name' in controller && + typeof controller.name === 'string' && + 'state' in controller && + typeof controller.state === 'object' && + controller instanceof BaseController + ); } export type ComposableControllerState = { - [name: string]: ControllerInstance['state']; + // `any` is used here to disable the `BaseController` type constraint which expects state properties to extend `Record`. + // `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; }; export type ComposableControllerStateChangeEvent = ControllerStateChangeEvent< typeof controllerName, - ComposableControllerState + Record >; export type ComposableControllerEvents = ComposableControllerStateChangeEvent; +type AnyControllerStateChangeEvent = ControllerStateChangeEvent< + string, + Record +>; + +type AllowedEvents = AnyControllerStateChangeEvent; + export type ComposableControllerMessenger = RestrictedControllerMessenger< typeof controllerName, never, - ControllerStateChangeEvent>, - string, - string + ComposableControllerEvents | AllowedEvents, + never, + AllowedEvents['type'] >; /** @@ -63,8 +120,6 @@ export class ComposableController extends BaseController< ComposableControllerState, ComposableControllerMessenger > { - readonly #controllers: ControllerList = []; - /** * Creates a ComposableController instance. * @@ -77,7 +132,7 @@ export class ComposableController extends BaseController< controllers, messenger, }: { - controllers: ControllerList; + controllers: ControllerInstance[]; messenger: ComposableControllerMessenger; }) { if (messenger === undefined) { @@ -86,23 +141,33 @@ export class ComposableController extends BaseController< super({ name: controllerName, - metadata: {}, - state: controllers.reduce((state, controller) => { - return { ...state, [controller.name]: controller.state }; - }, {} as ComposableControllerState), + metadata: controllers.reduce>( + (metadata, controller) => ({ + ...metadata, + [controller.name]: isBaseController(controller) + ? controller.metadata + : { persist: true, anonymous: true }, + }), + {}, + ), + state: controllers.reduce( + (state, controller) => { + return { ...state, [controller.name]: controller.state }; + }, + {}, + ), messenger, }); - this.#controllers = controllers; - this.#controllers.forEach((controller) => + controllers.forEach((controller) => this.#updateChildController(controller), ); } /** - * Adds a child controller instance to composable controller state, - * or updates the state of a child controller. + * 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 { const { name } = controller; @@ -113,15 +178,18 @@ export class ComposableController extends BaseController< [name]: childState, })); }); - } else { - this.messagingSystem.subscribe( - `${String(name)}:stateChange`, - (childState: Record) => { + } else if (isBaseController(controller)) { + this.messagingSystem.subscribe(`${name}:stateChange`, (childState) => { + if (isValidJson(childState)) { this.update((state) => ({ ...state, [name]: childState, })); - }, + } + }); + } else { + throw new Error( + 'Invalid controller: controller must extend from BaseController or BaseControllerV1', ); } } diff --git a/packages/composable-controller/src/index.ts b/packages/composable-controller/src/index.ts index da2d27e177e..c803f27d442 100644 --- a/packages/composable-controller/src/index.ts +++ b/packages/composable-controller/src/index.ts @@ -1,8 +1,14 @@ export type { - ControllerList, + BaseControllerV1Instance, + BaseControllerV2Instance, + ControllerInstance, ComposableControllerState, ComposableControllerStateChangeEvent, ComposableControllerEvents, ComposableControllerMessenger, } from './ComposableController'; -export { ComposableController } from './ComposableController'; +export { + ComposableController, + isBaseController, + isBaseControllerV1, +} from './ComposableController'; diff --git a/packages/composable-controller/tsconfig.json b/packages/composable-controller/tsconfig.json index f2d7b67ff66..cc814f313b7 100644 --- a/packages/composable-controller/tsconfig.json +++ b/packages/composable-controller/tsconfig.json @@ -6,6 +6,9 @@ "references": [ { "path": "../base-controller" + }, + { + "path": "../json-rpc-engine" } ], "include": ["../../types", "./src"] diff --git a/yarn.lock b/yarn.lock index fcc17862f20..17c75c30e27 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1708,6 +1708,8 @@ __metadata: dependencies: "@metamask/auto-changelog": ^3.4.4 "@metamask/base-controller": ^4.1.1 + "@metamask/json-rpc-engine": ^7.3.2 + "@metamask/utils": ^8.3.0 "@types/jest": ^27.4.1 deepmerge: ^4.2.2 immer: ^9.0.6