diff --git a/packages/base-controller/src/BaseControllerV2.ts b/packages/base-controller/src/BaseControllerV2.ts index 66d5ad02eb..5adb01b9df 100644 --- a/packages/base-controller/src/BaseControllerV2.ts +++ b/packages/base-controller/src/BaseControllerV2.ts @@ -113,13 +113,6 @@ export class BaseController< public readonly metadata: StateMetadata; - /** - * The existence of the `subscribe` property is how the ComposableController detects whether a - * controller extends the old BaseController or the new one. We set it to `undefined` here to - * ensure the ComposableController never mistakes them for an older style controller. - */ - public readonly subscribe: undefined; - /** * Creates a BaseController instance. * diff --git a/packages/composable-controller/CHANGELOG.md b/packages/composable-controller/CHANGELOG.md index b759c02157..c53cb5ad00 100644 --- a/packages/composable-controller/CHANGELOG.md +++ b/packages/composable-controller/CHANGELOG.md @@ -5,6 +5,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- Add types `ComposableControllerState`, `ComposableControllerStateChangeEvent`, `ComposableControllerEvents`, `ComposableControllerMessenger` ([#3590](https://github.com/MetaMask/core/pull/3590)) + +### Changed +- **BREAKING:** `ComposableController` is upgraded to extend `BaseControllerV2` ([#3590](https://github.com/MetaMask/core/pull/3590)) + - The constructor now expects an options object with required properties `controllers` and `messenger` as its only argument. + - `ComposableController` no longer has a `subscribe` method. Instead, listeners for `ComposableController` events must be registered to the controller messenger that generated the restricted messenger assigned to the instance's `messagingSystem` class field. + - Any getters for `ComposableController` state that access the internal class field directly should be refactored to instead use listeners that are subscribed to `ComposableControllerStateChangeEvent`. ## [4.0.0] ### Changed diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index 4c141633d9..6bed303a76 100644 --- a/packages/composable-controller/src/ComposableController.test.ts +++ b/packages/composable-controller/src/ComposableController.test.ts @@ -10,6 +10,7 @@ import { import type { Patch } from 'immer'; import * as sinon from 'sinon'; +import type { ComposableControllerStateChangeEvent } from './ComposableController'; import { ComposableController } from './ComposableController'; // Mock BaseController classes @@ -26,7 +27,7 @@ type FooMessenger = RestrictedControllerMessenger< 'FooController', never, FooControllerEvent, - string, + never, never >; @@ -105,15 +106,13 @@ describe('ComposableController', () => { describe('BaseControllerV1', () => { it('should compose controller state', () => { - const composableMessenger = new ControllerMessenger().getRestricted< - 'ComposableController', - never, - never - >({ name: 'ComposableController' }); - const controller = new ComposableController( - [new BarController(), new BazController()], - composableMessenger, - ); + const composableMessenger = new ControllerMessenger().getRestricted({ + name: 'ComposableController', + }); + const controller = new ComposableController({ + controllers: [new BarController(), new BazController()], + messenger: composableMessenger, + }); expect(controller.state).toStrictEqual({ BarController: { bar: 'bar' }, @@ -122,15 +121,13 @@ describe('ComposableController', () => { }); it('should compose flat controller state', () => { - const composableMessenger = new ControllerMessenger().getRestricted< - 'ComposableController', - never, - never - >({ name: 'ComposableController' }); - const controller = new ComposableController( - [new BarController(), new BazController()], - composableMessenger, - ); + const composableMessenger = new ControllerMessenger().getRestricted({ + name: 'ComposableController', + }); + const controller = new ComposableController({ + controllers: [new BarController(), new BazController()], + messenger: composableMessenger, + }); expect(controller.flatState).toStrictEqual({ bar: 'bar', @@ -139,19 +136,23 @@ describe('ComposableController', () => { }); it('should notify listeners of nested state change', () => { - const composableMessenger = new ControllerMessenger().getRestricted< - 'ComposableController', + const controllerMessenger = new ControllerMessenger< never, - never - >({ name: 'ComposableController' }); + ComposableControllerStateChangeEvent + >(); + const composableMessenger = controllerMessenger.getRestricted({ + name: 'ComposableController', + }); const barController = new BarController(); - const controller = new ComposableController( - [barController], - composableMessenger, - ); + new ComposableController({ + controllers: [barController], + messenger: composableMessenger, + }); const listener = sinon.stub(); - controller.subscribe(listener); - + controllerMessenger.subscribe( + 'ComposableController:stateChange', + listener, + ); barController.updateBar('something different'); expect(listener.calledOnce).toBe(true); @@ -169,27 +170,19 @@ describe('ComposableController', () => { never, FooControllerEvent >(); - const fooControllerMessenger = controllerMessenger.getRestricted< - 'FooController', - never, - never - >({ + const fooControllerMessenger = controllerMessenger.getRestricted({ name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); - const composableControllerMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - 'FooController:stateChange' - >({ + const composableControllerMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', allowedEvents: ['FooController:stateChange'], }); - const composableController = new ComposableController( - [fooController], - composableControllerMessenger, - ); + const composableController = new ComposableController({ + controllers: [fooController], + messenger: composableControllerMessenger, + }); expect(composableController.state).toStrictEqual({ FooController: { foo: 'foo' }, }); @@ -200,26 +193,18 @@ describe('ComposableController', () => { never, FooControllerEvent >(); - const fooControllerMessenger = controllerMessenger.getRestricted< - 'FooController', - never, - never - >({ + const fooControllerMessenger = controllerMessenger.getRestricted({ name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); - const composableControllerMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - 'FooController:stateChange' - >({ + const composableControllerMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', allowedEvents: ['FooController:stateChange'], }); - const composableController = new ComposableController( - [fooController], - composableControllerMessenger, - ); + const composableController = new ComposableController({ + controllers: [fooController], + messenger: composableControllerMessenger, + }); expect(composableController.flatState).toStrictEqual({ foo: 'foo', }); @@ -228,7 +213,7 @@ describe('ComposableController', () => { it('should notify listeners of nested state change', () => { const controllerMessenger = new ControllerMessenger< never, - FooControllerEvent + ComposableControllerStateChangeEvent | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -238,21 +223,20 @@ describe('ComposableController', () => { name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); - const composableControllerMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - 'FooController:stateChange' - >({ + const composableControllerMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', allowedEvents: ['FooController:stateChange'], }); - const composableController = new ComposableController( - [fooController], - composableControllerMessenger, - ); + new ComposableController({ + controllers: [fooController], + messenger: composableControllerMessenger, + }); const listener = sinon.stub(); - composableController.subscribe(listener); + controllerMessenger.subscribe( + 'ComposableController:stateChange', + listener, + ); fooController.updateFoo('bar'); expect(listener.calledOnce).toBe(true); @@ -271,26 +255,18 @@ describe('ComposableController', () => { never, FooControllerEvent >(); - const fooControllerMessenger = controllerMessenger.getRestricted< - 'FooController', - never, - never - >({ + const fooControllerMessenger = controllerMessenger.getRestricted({ name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); - const composableControllerMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - 'FooController:stateChange' - >({ + const composableControllerMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', allowedEvents: ['FooController:stateChange'], }); - const composableController = new ComposableController( - [barController, fooController], - composableControllerMessenger, - ); + const composableController = new ComposableController({ + controllers: [barController, fooController], + messenger: composableControllerMessenger, + }); expect(composableController.state).toStrictEqual({ BarController: { bar: 'bar' }, FooController: { foo: 'foo' }, @@ -303,26 +279,18 @@ describe('ComposableController', () => { never, FooControllerEvent >(); - const fooControllerMessenger = controllerMessenger.getRestricted< - 'FooController', - never, - never - >({ + const fooControllerMessenger = controllerMessenger.getRestricted({ name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); - const composableControllerMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - 'FooController:stateChange' - >({ + const composableControllerMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', allowedEvents: ['FooController:stateChange'], }); - const composableController = new ComposableController( - [barController, fooController], - composableControllerMessenger, - ); + const composableController = new ComposableController({ + controllers: [barController, fooController], + messenger: composableControllerMessenger, + }); expect(composableController.flatState).toStrictEqual({ bar: 'bar', foo: 'foo', @@ -333,7 +301,7 @@ describe('ComposableController', () => { const barController = new BarController(); const controllerMessenger = new ControllerMessenger< never, - FooControllerEvent + ComposableControllerStateChangeEvent | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -343,21 +311,19 @@ describe('ComposableController', () => { name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); - const composableControllerMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - 'FooController:stateChange' - >({ + const composableControllerMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', allowedEvents: ['FooController:stateChange'], }); - const composableController = new ComposableController( - [barController, fooController], - composableControllerMessenger, - ); - + new ComposableController({ + controllers: [barController, fooController], + messenger: composableControllerMessenger, + }); const listener = sinon.stub(); - composableController.subscribe(listener); + controllerMessenger.subscribe( + 'ComposableController:stateChange', + listener, + ); barController.updateBar('foo'); expect(listener.calledOnce).toBe(true); @@ -375,7 +341,7 @@ describe('ComposableController', () => { const barController = new BarController(); const controllerMessenger = new ControllerMessenger< never, - FooControllerEvent + ComposableControllerStateChangeEvent | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -385,21 +351,20 @@ describe('ComposableController', () => { name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); - const composableControllerMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - 'FooController:stateChange' - >({ + const composableControllerMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', allowedEvents: ['FooController:stateChange'], }); - const composableController = new ComposableController( - [barController, fooController], - composableControllerMessenger, - ); + new ComposableController({ + controllers: [barController, fooController], + messenger: composableControllerMessenger, + }); const listener = sinon.stub(); - composableController.subscribe(listener); + controllerMessenger.subscribe( + 'ComposableController:stateChange', + listener, + ); fooController.updateFoo('bar'); expect(listener.calledOnce).toBe(true); @@ -419,19 +384,17 @@ describe('ComposableController', () => { never, FooControllerEvent >(); - const fooControllerMessenger = controllerMessenger.getRestricted< - 'FooController', - never, - never - >({ + const fooControllerMessenger = controllerMessenger.getRestricted({ name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); expect( - () => new ComposableController([barController, fooController]), - ).toThrow( - 'Messaging system required if any BaseController controllers are used', - ); + () => + // @ts-expect-error - Suppressing type error to test for runtime error handling + new ComposableController({ + controllers: [barController, fooController], + }), + ).toThrow('Messaging system is required'); }); }); }); diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index bc599e0028..3088d68049 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -1,75 +1,99 @@ -import type { RestrictedControllerMessenger } from '@metamask/base-controller'; -import { BaseControllerV1 } from '@metamask/base-controller'; +import { BaseController, BaseControllerV1 } from '@metamask/base-controller'; +import type { + ControllerStateChangeEvent, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; -/** - * List of child controller instances - * - * This type encompasses controllers based up either BaseControllerV1 or +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). */ -export type ControllerList = ( +type ControllerInstance = | BaseControllerV1 - | { name: string; state: Record } -)[]; + | { name: string; state: Record }; -export type ComposableControllerRestrictedMessenger = - RestrictedControllerMessenger<'ComposableController', never, any, never, any>; +/** + * List of child controller instances + */ +export type ControllerList = ControllerInstance[]; /** - * Controller that can be used to compose multiple controllers together. + * 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 */ -export class ComposableController extends BaseControllerV1 { - private readonly controllers: ControllerList = []; +function isBaseControllerV1( + controller: ControllerInstance, +): controller is BaseControllerV1 { + return controller instanceof BaseControllerV1; +} - private readonly messagingSystem?: ComposableControllerRestrictedMessenger; +export type ComposableControllerState = { + [name: string]: ControllerInstance['state']; +}; - /** - * Name of this controller used during composition - */ - override name = 'ComposableController'; +export type ComposableControllerStateChangeEvent = ControllerStateChangeEvent< + typeof controllerName, + ComposableControllerState +>; + +export type ComposableControllerEvents = ComposableControllerStateChangeEvent; + +export type ComposableControllerMessenger = RestrictedControllerMessenger< + typeof controllerName, + never, + ControllerStateChangeEvent>, + string, + string +>; + +/** + * Controller that can be used to compose multiple controllers together. + */ +export class ComposableController extends BaseController< + typeof controllerName, + ComposableControllerState, + ComposableControllerMessenger +> { + readonly #controllers: ControllerList = []; /** * Creates a ComposableController instance. * - * @param controllers - Map of names to controller instances. - * @param messenger - The controller messaging system, used for communicating with BaseController controllers. + * @param options - Initial options used to configure this controller + * @param options.controllers - List of child controller instances to compose. + * @param options.messenger - A restricted controller messenger. */ - constructor( - controllers: ControllerList, - messenger?: ComposableControllerRestrictedMessenger, - ) { - super( - undefined, - controllers.reduce((state, controller) => { - state[controller.name] = controller.state; - return state; - }, {} as any), - ); - this.initialize(); - this.controllers = controllers; - this.messagingSystem = messenger; - this.controllers.forEach((controller) => { - const { name } = controller; - if ((controller as BaseControllerV1).subscribe !== undefined) { - (controller as BaseControllerV1).subscribe((state) => { - this.update({ [name]: state }); - }); - } else if (this.messagingSystem) { - (this.messagingSystem.subscribe as any)( - `${name}:stateChange`, - (state: any) => { - this.update({ [name]: state }); - }, - ); - } else { - throw new Error( - `Messaging system required if any BaseController controllers are used`, - ); - } + + constructor({ + controllers, + messenger, + }: { + controllers: ControllerList; + messenger: ComposableControllerMessenger; + }) { + if (messenger === undefined) { + throw new Error(`Messaging system is required`); + } + + super({ + name: controllerName, + metadata: {}, + state: controllers.reduce((state, controller) => { + return { ...state, [controller.name]: controller.state }; + }, {} as ComposableControllerState), + messenger, }); + + this.#controllers = controllers; + this.#controllers.forEach((controller) => + this.#updateChildController(controller), + ); } /** @@ -81,11 +105,38 @@ export class ComposableController extends BaseControllerV1 { */ get flatState() { let flatState = {}; - for (const controller of this.controllers) { + for (const controller of this.#controllers) { flatState = { ...flatState, ...controller.state }; } return flatState; } + + /** + * Adds a child controller instance to composable controller state, + * or updates the state of a child controller. + * @param controller - Controller instance to update + */ + #updateChildController(controller: ControllerInstance): void { + const { name } = controller; + if (isBaseControllerV1(controller)) { + controller.subscribe((childState) => { + this.update((state) => ({ + ...state, + [name]: childState, + })); + }); + } else { + this.messagingSystem.subscribe( + `${String(name)}:stateChange`, + (childState: Record) => { + this.update((state) => ({ + ...state, + [name]: childState, + })); + }, + ); + } + } } export default ComposableController; diff --git a/packages/composable-controller/src/index.ts b/packages/composable-controller/src/index.ts index b1a9e5a4b1..da2d27e177 100644 --- a/packages/composable-controller/src/index.ts +++ b/packages/composable-controller/src/index.ts @@ -1 +1,8 @@ -export * from './ComposableController'; +export type { + ControllerList, + ComposableControllerState, + ComposableControllerStateChangeEvent, + ComposableControllerEvents, + ComposableControllerMessenger, +} from './ComposableController'; +export { ComposableController } from './ComposableController';