From 63c29fbaca9b69ac4322d6701bae0cb7510cdbee Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 11:26:08 -0800 Subject: [PATCH 01/14] Define `ControllerInstance` type --- .../src/ComposableController.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index bc599e00282..b59c3d5fe79 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -3,20 +3,22 @@ import { BaseControllerV1 } from '@metamask/base-controller'; /** * List of child controller instances - * +/* * This type encompasses controllers based up 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 }; + +/** + * List of child controller instances + */ +export type ControllerList = ControllerInstance[]; -export type ComposableControllerRestrictedMessenger = - RestrictedControllerMessenger<'ComposableController', never, any, never, any>; /** * Controller that can be used to compose multiple controllers together. From 1e79aeeae61350326d2d678e6fff97cbf41782c8 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 11:28:02 -0800 Subject: [PATCH 02/14] Define types required by `BaseControllerV2` --- .../src/ComposableController.ts | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index b59c3d5fe79..259e567068a 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -1,8 +1,12 @@ -import type { RestrictedControllerMessenger } from '@metamask/base-controller'; -import { BaseControllerV1 } from '@metamask/base-controller'; +import type { + ControllerGetStateAction, + ControllerStateChangeEvent, + RestrictedControllerMessenger, + BaseControllerV1, +} from '@metamask/base-controller'; + +const controllerName = 'ComposableController'; -/** - * List of child controller instances /* * This type encompasses controllers based up either BaseControllerV1 or * BaseController. The BaseController type can't be included directly @@ -20,6 +24,32 @@ type ControllerInstance = export type ControllerList = ControllerInstance[]; +export type ComposableControllerState = { + [name: string]: ControllerInstance['state']; +}; + +export type ComposableControllerGetStateAction = ControllerGetStateAction< + `${typeof controllerName}`, + ComposableControllerState +>; + +export type ComposableControllerStateChangeEvent = ControllerStateChangeEvent< + `${typeof controllerName}`, + ComposableControllerState +>; + +export type ComposableControllerActions = ComposableControllerGetStateAction; + +export type ComposableControllerEvents = ComposableControllerStateChangeEvent; + +export type ComposableControllerMessenger = RestrictedControllerMessenger< + typeof controllerName, + ControllerGetStateAction>, + ControllerStateChangeEvent>, + string, + string +>; + /** * Controller that can be used to compose multiple controllers together. */ From d7d60652d5318e6421c9b7099e0bb10b5156489c Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 11:30:16 -0800 Subject: [PATCH 03/14] Extract `#updateChildController` method --- .../src/ComposableController.ts | 64 ++++++++++++------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index 259e567068a..26bec335079 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -23,6 +23,16 @@ type ControllerInstance = */ export type ControllerList = ControllerInstance[]; +/** + * 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 + */ +function isBaseControllerV1( + controller: ControllerInstance, +): controller is BaseControllerV1 { + return 'subscribe' in controller && controller.subscribe !== undefined; +} export type ComposableControllerState = { [name: string]: ControllerInstance['state']; @@ -78,30 +88,11 @@ export class ComposableController extends BaseControllerV1 { controllers.reduce((state, controller) => { state[controller.name] = controller.state; return state; - }, {} as any), + + this.#controllers = controllers; + this.#controllers.forEach((controller) => + this.#updateChildController(controller), ); - 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`, - ); - } - }); } /** @@ -118,6 +109,33 @@ export class ComposableController extends BaseControllerV1 { } 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; From 729f027f93804bb0c8cb58d9a065c3eed933bca4 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 11:30:52 -0800 Subject: [PATCH 04/14] Upgrade `ComposableController` to `BaseControllerV2` --- .../src/ComposableController.ts | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index 26bec335079..ea3e58e51f6 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -1,3 +1,4 @@ +import { BaseController } from '@metamask/base-controller'; import type { ControllerGetStateAction, ControllerStateChangeEvent, @@ -63,31 +64,42 @@ export type ComposableControllerMessenger = RestrictedControllerMessenger< /** * Controller that can be used to compose multiple controllers together. */ -export class ComposableController extends BaseControllerV1 { - private readonly controllers: ControllerList = []; - - private readonly messagingSystem?: ComposableControllerRestrictedMessenger; - - /** - * Name of this controller used during composition - */ - override name = 'ComposableController'; +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 controllers - List of controller instances. * @param messenger - The controller messaging system, used for communicating with BaseController controllers. */ - constructor( - controllers: ControllerList, - messenger?: ComposableControllerRestrictedMessenger, - ) { - super( - undefined, - controllers.reduce((state, controller) => { + + constructor({ + controllers, + messenger, + }: { + controllers: ControllerList; + messenger: ComposableControllerMessenger; + }) { + if (messenger === undefined) { + throw new Error( + `Messaging system required if any BaseController controllers are used`, + ); + } + + super({ + name: controllerName, + metadata: {}, + state: controllers.reduce((state, controller) => { state[controller.name] = controller.state; return state; + }, {} as ComposableControllerState), + messenger, + }); this.#controllers = controllers; this.#controllers.forEach((controller) => @@ -104,7 +116,7 @@ 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; From 84d96e1b0ad33006adb00c9ffb28d8247f9cd9aa Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 11:32:01 -0800 Subject: [PATCH 05/14] Supress type error in test for missing messenger param --- .../composable-controller/src/ComposableController.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index 4c141633d91..c94ec49bbe7 100644 --- a/packages/composable-controller/src/ComposableController.test.ts +++ b/packages/composable-controller/src/ComposableController.test.ts @@ -428,7 +428,11 @@ describe('ComposableController', () => { }); const fooController = new FooController(fooControllerMessenger); expect( - () => new ComposableController([barController, fooController]), + () => + // @ts-expect-error - Suppressing type error to test for runtime error handling + new ComposableController({ + controllers: [barController, fooController], + }), ).toThrow( 'Messaging system required if any BaseController controllers are used', ); From 7cd88fb536272e1c8cb91f83a718694a5df2e538 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 11:53:42 -0800 Subject: [PATCH 06/14] Fix tests to align with upgraded `ComposableController` API --- .../src/ComposableController.test.ts | 141 ++++++++++-------- 1 file changed, 77 insertions(+), 64 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index c94ec49bbe7..9b7c5c7d115 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 >; @@ -109,11 +110,13 @@ describe('ComposableController', () => { 'ComposableController', never, never - >({ name: 'ComposableController' }); - const controller = new ComposableController( - [new BarController(), new BazController()], - composableMessenger, - ); + >({ + name: 'ComposableController', + }); + const controller = new ComposableController({ + controllers: [new BarController(), new BazController()], + messenger: composableMessenger, + }); expect(controller.state).toStrictEqual({ BarController: { bar: 'bar' }, @@ -126,11 +129,13 @@ describe('ComposableController', () => { 'ComposableController', never, never - >({ name: 'ComposableController' }); - const controller = new ComposableController( - [new BarController(), new BazController()], - composableMessenger, - ); + >({ + name: 'ComposableController', + }); + const controller = new ComposableController({ + controllers: [new BarController(), new BazController()], + messenger: composableMessenger, + }); expect(controller.flatState).toStrictEqual({ bar: 'bar', @@ -139,19 +144,27 @@ describe('ComposableController', () => { }); it('should notify listeners of nested state change', () => { - const composableMessenger = new ControllerMessenger().getRestricted< + const controllerMessenger = new ControllerMessenger< + never, + ComposableControllerStateChangeEvent + >(); + const composableMessenger = controllerMessenger.getRestricted< 'ComposableController', never, never - >({ name: 'ComposableController' }); + >({ + 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 +182,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' }, }); @@ -216,10 +221,10 @@ describe('ComposableController', () => { 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 +233,7 @@ describe('ComposableController', () => { it('should notify listeners of nested state change', () => { const controllerMessenger = new ControllerMessenger< never, - FooControllerEvent + ComposableControllerStateChangeEvent | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -246,13 +251,16 @@ describe('ComposableController', () => { 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); @@ -287,10 +295,10 @@ describe('ComposableController', () => { 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' }, @@ -319,10 +327,10 @@ describe('ComposableController', () => { 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 +341,7 @@ describe('ComposableController', () => { const barController = new BarController(); const controllerMessenger = new ControllerMessenger< never, - FooControllerEvent + ComposableControllerStateChangeEvent | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -351,13 +359,15 @@ describe('ComposableController', () => { 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 +385,7 @@ describe('ComposableController', () => { const barController = new BarController(); const controllerMessenger = new ControllerMessenger< never, - FooControllerEvent + ComposableControllerStateChangeEvent | FooControllerEvent >(); const fooControllerMessenger = controllerMessenger.getRestricted< 'FooController', @@ -393,13 +403,16 @@ describe('ComposableController', () => { 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); From 4c42fe999059f1a2c28213be483bbc683d664864 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 11:54:39 -0800 Subject: [PATCH 07/14] Omit inferrable generic params in `.getRestricted()` calls --- .../src/ComposableController.test.ts | 78 ++++--------------- 1 file changed, 13 insertions(+), 65 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index 9b7c5c7d115..f5be937ed47 100644 --- a/packages/composable-controller/src/ComposableController.test.ts +++ b/packages/composable-controller/src/ComposableController.test.ts @@ -106,11 +106,7 @@ describe('ComposableController', () => { describe('BaseControllerV1', () => { it('should compose controller state', () => { - const composableMessenger = new ControllerMessenger().getRestricted< - 'ComposableController', - never, - never - >({ + const composableMessenger = new ControllerMessenger().getRestricted({ name: 'ComposableController', }); const controller = new ComposableController({ @@ -125,11 +121,7 @@ describe('ComposableController', () => { }); it('should compose flat controller state', () => { - const composableMessenger = new ControllerMessenger().getRestricted< - 'ComposableController', - never, - never - >({ + const composableMessenger = new ControllerMessenger().getRestricted({ name: 'ComposableController', }); const controller = new ComposableController({ @@ -148,11 +140,7 @@ describe('ComposableController', () => { never, ComposableControllerStateChangeEvent >(); - const composableMessenger = controllerMessenger.getRestricted< - 'ComposableController', - never, - never - >({ + const composableMessenger = controllerMessenger.getRestricted({ name: 'ComposableController', }); const barController = new BarController(); @@ -205,19 +193,11 @@ 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'], }); @@ -243,11 +223,7 @@ 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'], }); @@ -279,19 +255,11 @@ 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'], }); @@ -311,19 +279,11 @@ 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'], }); @@ -351,11 +311,7 @@ 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'], }); @@ -395,11 +351,7 @@ 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'], }); @@ -432,11 +384,7 @@ describe('ComposableController', () => { never, FooControllerEvent >(); - const fooControllerMessenger = controllerMessenger.getRestricted< - 'FooController', - never, - never - >({ + const fooControllerMessenger = controllerMessenger.getRestricted({ name: 'FooController', }); const fooController = new FooController(fooControllerMessenger); From 1959df15cc402ddaa45b7b1542a313503e8135de Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Nov 2023 12:36:08 -0800 Subject: [PATCH 08/14] Add JSDoc for constructor params --- packages/composable-controller/src/ComposableController.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index ea3e58e51f6..3e7bf4ce77f 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -74,8 +74,9 @@ export class ComposableController extends BaseController< /** * Creates a ComposableController instance. * - * @param controllers - List of 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({ From 65839dc1eacaa4d563bf814369bb5df1a3a3d47a Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 4 Dec 2023 15:15:35 -0800 Subject: [PATCH 09/14] Apply review suggestions Co-authored-by: Ellilot Winkler --- packages/composable-controller/src/ComposableController.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index 3e7bf4ce77f..cc5349cdc02 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -9,7 +9,7 @@ import type { const controllerName = 'ComposableController'; /* - * This type encompasses controllers based up either BaseControllerV1 or + * 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 @@ -45,7 +45,7 @@ export type ComposableControllerGetStateAction = ControllerGetStateAction< >; export type ComposableControllerStateChangeEvent = ControllerStateChangeEvent< - `${typeof controllerName}`, + typeof controllerName, ComposableControllerState >; @@ -96,8 +96,7 @@ export class ComposableController extends BaseController< name: controllerName, metadata: {}, state: controllers.reduce((state, controller) => { - state[controller.name] = controller.state; - return state; + return { ...state, [controller.name]: controller.state }; }, {} as ComposableControllerState), messenger, }); From 63d5d064f3c8fb0cb7cf2b6859c88ebf0ebe32e6 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 4 Dec 2023 15:17:32 -0800 Subject: [PATCH 10/14] Rewrite `BaseControllerV1` assertion logic to use `instanceOf`, remove`BaseControllerV2.subscribe` field --- packages/base-controller/src/BaseControllerV2.ts | 7 ------- packages/composable-controller/src/ComposableController.ts | 6 ++---- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/base-controller/src/BaseControllerV2.ts b/packages/base-controller/src/BaseControllerV2.ts index 66d5ad02eba..5adb01b9df4 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/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index cc5349cdc02..c2d4b560cde 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -1,9 +1,7 @@ -import { BaseController } from '@metamask/base-controller'; +import { BaseController, BaseControllerV1 } from '@metamask/base-controller'; import type { - ControllerGetStateAction, ControllerStateChangeEvent, RestrictedControllerMessenger, - BaseControllerV1, } from '@metamask/base-controller'; const controllerName = 'ComposableController'; @@ -32,7 +30,7 @@ export type ControllerList = ControllerInstance[]; function isBaseControllerV1( controller: ControllerInstance, ): controller is BaseControllerV1 { - return 'subscribe' in controller && controller.subscribe !== undefined; + return controller instanceof BaseControllerV1; } export type ComposableControllerState = { From ac750ec7f5a38489ddbad340d2ce0322d01fa1d3 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 4 Dec 2023 15:19:00 -0800 Subject: [PATCH 11/14] Always require messenger in constructor - No other way to handle `super({ messenger })` call now that `ComposableController` extends from `BaseControllerV2` --- .../composable-controller/src/ComposableController.test.ts | 4 +--- packages/composable-controller/src/ComposableController.ts | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index f5be937ed47..6bed303a762 100644 --- a/packages/composable-controller/src/ComposableController.test.ts +++ b/packages/composable-controller/src/ComposableController.test.ts @@ -394,9 +394,7 @@ describe('ComposableController', () => { new ComposableController({ controllers: [barController, fooController], }), - ).toThrow( - 'Messaging system required if any BaseController controllers are used', - ); + ).toThrow('Messaging system is required'); }); }); }); diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index c2d4b560cde..ac73027826b 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -85,9 +85,7 @@ export class ComposableController extends BaseController< messenger: ComposableControllerMessenger; }) { if (messenger === undefined) { - throw new Error( - `Messaging system required if any BaseController controllers are used`, - ); + throw new Error(`Messaging system is required`); } super({ From 1ddf33cd10ec33a4c0a375b4749b0d2bbe87946e Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 4 Dec 2023 15:20:19 -0800 Subject: [PATCH 12/14] Remove `ComposableController{GetStateAction,Actions}` --- .../composable-controller/src/ComposableController.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index ac73027826b..64e2f94a163 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -37,23 +37,16 @@ export type ComposableControllerState = { [name: string]: ControllerInstance['state']; }; -export type ComposableControllerGetStateAction = ControllerGetStateAction< - `${typeof controllerName}`, - ComposableControllerState ->; - export type ComposableControllerStateChangeEvent = ControllerStateChangeEvent< typeof controllerName, ComposableControllerState >; -export type ComposableControllerActions = ComposableControllerGetStateAction; - export type ComposableControllerEvents = ComposableControllerStateChangeEvent; export type ComposableControllerMessenger = RestrictedControllerMessenger< typeof controllerName, - ControllerGetStateAction>, + never, ControllerStateChangeEvent>, string, string From cdfe5402876f670569f435132d3b003b6571ebf4 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 4 Dec 2023 15:20:33 -0800 Subject: [PATCH 13/14] Explicitly enumerate package-level exports --- .../composable-controller/src/ComposableController.ts | 2 +- packages/composable-controller/src/index.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index 64e2f94a163..3088d680493 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -4,7 +4,7 @@ import type { RestrictedControllerMessenger, } from '@metamask/base-controller'; -const controllerName = 'ComposableController'; +export const controllerName = 'ComposableController'; /* * This type encompasses controllers based on either BaseControllerV1 or diff --git a/packages/composable-controller/src/index.ts b/packages/composable-controller/src/index.ts index b1a9e5a4b14..da2d27e177e 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'; From 55ed68696aab652333b9459121c5572a800b8795 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 5 Dec 2023 08:13:38 -0800 Subject: [PATCH 14/14] Record CHANGELOG entries --- packages/composable-controller/CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/composable-controller/CHANGELOG.md b/packages/composable-controller/CHANGELOG.md index b759c021577..c53cb5ad004 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