[composable-controller] Subscribe to stateChange events of V1 contr…

…ollers with messenger, type fixes (#3964) ## Explanation - If a controller is V1 but it has a messaging system, subscribe to its `stateChange` event. - Remove `@metamask/utils` as a dependency. - Remove package-level exports for `BaseControllerV{1,2}Instance`, `ControllerInstance`, as these are incomplete and unsuited for general usage. `isBaseController{,V1}` are not removed. - Rename `BaseControllerV2Instance` as `BaseControllerInstance` for consistency with `isBaseController`. ## References - Closes #3966 ## Changelog ### [`@metamask/composable-controller`](https://github.com/MetaMask/core/pull/3964/files#diff-9745631a8a7023c408b4f9b96dc8c0eaa9a41599e8d93eb470bb268f1fcc75ff) ## 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 --------- Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MetaMask · Mar 7, 2024 · 964e389 · 964e389
1 parent 4b01dc3
commit 964e389
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 57 deletions.
diff --git a/packages/composable-controller/CHANGELOG.md b/packages/composable-controller/CHANGELOG.md
@@ -10,13 +10,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### 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))
+- Subscribe to the `stateChange` events of `BaseControllerV1` controllers that have a `messagingSystem` class field with an assigned instance of the `RestrictedControllerMessenger` class. ([#3964](https://github.com/MetaMask/core/pull/3964))
 
 ### Removed
 

diff --git a/packages/composable-controller/package.json b/packages/composable-controller/package.json
@@ -31,8 +31,7 @@
     "test:watch": "jest --watch"
   },
   "dependencies": {
-    "@metamask/base-controller": "^4.1.1",
-    "@metamask/utils": "^8.3.0"
+    "@metamask/base-controller": "^4.1.1"
   },
   "devDependencies": {
     "@metamask/auto-changelog": "^3.4.4",

diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts
@@ -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(
         `${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 });
+        });
+      });
     }
   }
 }

diff --git a/packages/composable-controller/src/index.ts b/packages/composable-controller/src/index.ts
@@ -1,7 +1,4 @@
 export type {
-  BaseControllerV1Instance,
-  BaseControllerV2Instance,
-  ControllerInstance,
   ComposableControllerState,
   ComposableControllerStateChangeEvent,
   ComposableControllerEvents,

diff --git a/yarn.lock b/yarn.lock
@@ -1712,7 +1712,6 @@ __metadata:
     "@metamask/auto-changelog": ^3.4.4
     "@metamask/base-controller": ^4.1.1
     "@metamask/json-rpc-engine": ^7.3.3
-    "@metamask/utils": ^8.3.0
     "@types/jest": ^27.4.1
     deepmerge: ^4.2.2
     immer: ^9.0.6