diff --git a/packages/base-controller/src/BaseControllerV1.test.ts b/packages/base-controller/src/BaseControllerV1.test.ts index f268f0f4a3..3d52371f35 100644 --- a/packages/base-controller/src/BaseControllerV1.test.ts +++ b/packages/base-controller/src/BaseControllerV1.test.ts @@ -51,7 +51,6 @@ describe('isBaseControllerV1', () => { it('should return false if passed a non-controller', () => { const notController = new JsonRpcEngine(); - // @ts-expect-error Intentionally passing invalid input to test runtime behavior expect(isBaseControllerV1(notController)).toBe(false); }); }); diff --git a/packages/base-controller/src/BaseControllerV1.ts b/packages/base-controller/src/BaseControllerV1.ts index 97843f642e..1b68072415 100644 --- a/packages/base-controller/src/BaseControllerV1.ts +++ b/packages/base-controller/src/BaseControllerV1.ts @@ -1,6 +1,4 @@ -import type { PublicInterface } from '@metamask/utils'; - -import type { ControllerInstance } from './BaseControllerV2'; +import { isNullOrUndefined, type PublicInterface } from '@metamask/utils'; /** * Determines if the given controller is an instance of `BaseControllerV1` @@ -9,9 +7,11 @@ import type { ControllerInstance } from './BaseControllerV2'; * @returns True if the controller is an instance of `BaseControllerV1` */ export function isBaseControllerV1( - controller: ControllerInstance, + controller: unknown, ): controller is BaseControllerV1Instance { return ( + typeof controller === 'object' && + !isNullOrUndefined(controller) && 'name' in controller && typeof controller.name === 'string' && 'config' in controller && diff --git a/packages/base-controller/src/BaseControllerV2.test.ts b/packages/base-controller/src/BaseControllerV2.test.ts index 9227505090..a90ee1a46b 100644 --- a/packages/base-controller/src/BaseControllerV2.test.ts +++ b/packages/base-controller/src/BaseControllerV2.test.ts @@ -203,7 +203,6 @@ describe('isBaseController', () => { it('should return false if passed a non-controller', () => { const notController = new JsonRpcEngine(); - // @ts-expect-error Intentionally passing invalid input to test runtime behavior expect(isBaseController(notController)).toBe(false); }); }); diff --git a/packages/base-controller/src/BaseControllerV2.ts b/packages/base-controller/src/BaseControllerV2.ts index 355d156689..9639caa4a3 100644 --- a/packages/base-controller/src/BaseControllerV2.ts +++ b/packages/base-controller/src/BaseControllerV2.ts @@ -1,4 +1,8 @@ -import type { Json, PublicInterface } from '@metamask/utils'; +import { + isNullOrUndefined, + type Json, + type PublicInterface, +} from '@metamask/utils'; import { enablePatches, produceWithPatches, applyPatches, freeze } from 'immer'; import type { Draft, Patch } from 'immer'; @@ -21,9 +25,11 @@ enablePatches(); * @returns True if the controller is an instance of `BaseController` */ export function isBaseController( - controller: ControllerInstance, + controller: unknown, ): controller is BaseControllerInstance { return ( + typeof controller === 'object' && + !isNullOrUndefined(controller) && 'name' in controller && typeof controller.name === 'string' && 'state' in controller && @@ -130,7 +136,7 @@ export type StateMetadataConstraint = Record< >; /** - * The widest subtype of all controller instances that inherit from `BaseController` (formerly `BaseControllerV2`). + * The narrowest supertype of all controller instances that inherit from `BaseController` (formerly `BaseControllerV2`). * Any `BaseController` subclass instance can be assigned to this type. */ export type BaseControllerInstance = Omit< @@ -147,7 +153,7 @@ export type BaseControllerInstance = Omit< }; /** - * A widest subtype of all controller instances that inherit from `BaseController` (formerly `BaseControllerV2`) or `BaseControllerV1`. + * A narrowest supertype of all controller instances that inherit from `BaseController` (formerly `BaseControllerV2`) or `BaseControllerV1`. * Any `BaseController` or `BaseControllerV1` subclass instance can be assigned to this type. */ // TODO: Remove once BaseControllerV2 migrations are completed for all controllers. diff --git a/packages/composable-controller/src/ComposableController.test.ts b/packages/composable-controller/src/ComposableController.test.ts index e876559082..79b0fab4f1 100644 --- a/packages/composable-controller/src/ComposableController.test.ts +++ b/packages/composable-controller/src/ComposableController.test.ts @@ -15,10 +15,7 @@ import type { ChildControllerStateChangeEvents, ComposableControllerEvents, } from './ComposableController'; -import { - ComposableController, - INVALID_CONTROLLER_ERROR, -} from './ComposableController'; +import { ComposableController } from './ComposableController'; // Mock BaseController classes @@ -531,7 +528,7 @@ describe('ComposableController', () => { ).toThrow('Messaging system is required'); }); - it('should throw if composing a controller that does not extend from BaseController', () => { + it('should not throw if composing a controller that does not extend from BaseController', () => { type ComposableControllerState = { // TODO: Either fix this lint violation or explain why it's necessary to ignore. // eslint-disable-next-line @typescript-eslint/naming-convention @@ -564,7 +561,7 @@ describe('ComposableController', () => { controllers: [notController, fooController], messenger: composableControllerMessenger, }), - ).toThrow(INVALID_CONTROLLER_ERROR); + ).not.toThrow(); }); }); }); diff --git a/packages/composable-controller/src/ComposableController.ts b/packages/composable-controller/src/ComposableController.ts index e93cbde984..2e2cff3900 100644 --- a/packages/composable-controller/src/ComposableController.ts +++ b/packages/composable-controller/src/ComposableController.ts @@ -16,8 +16,32 @@ import type { Patch } from 'immer'; export const controllerName = 'ComposableController'; -export const INVALID_CONTROLLER_ERROR = - 'Invalid controller: controller must have a `messagingSystem` or be a class inheriting from `BaseControllerV1`.'; +// TODO: Replace with type guard once superclass for messageable non-controllers has been defined. +export const STATELESS_NONCONTROLLER_NAMES = [ + 'AssetsContractController', + 'NftDetectionController', + 'TokenDetectionController', +] as const; + +/** + * A type guard for the {@link ControllerInstance} type that specifically filters out non-controllers that have empty state. + * @param controller - The controller instance to check. + * @returns A type predicate that evaluates to true if the input is a {@link ControllerInstance} type. + */ +export function isController( + controller: unknown, +): controller is ControllerInstance { + return ( + (isBaseController(controller) || isBaseControllerV1(controller)) && + !STATELESS_NONCONTROLLER_NAMES.find((name) => name === controller.name) + ); +} + +/** + * A universal supertype for modules with a 'string'-type `name` property. + * This type is intended to encompass controller and non-controller input that can be passed into the {@link ComposableController} `controllers` constructor option. + */ +export type NamedModule = { name: string }; /** * A universal supertype for the composable controller state object. @@ -30,7 +54,7 @@ type LegacyComposableControllerStateConstraint = { // `any` is used here to disable the generic constraint on the `ControllerState` type argument in the `BaseController` type, // enabling composable controller state types with BaseControllerV1 state objects to be. // eslint-disable-next-line @typescript-eslint/no-explicit-any - [name: string]: Record; + [controllerName: string]: Record; }; /** @@ -39,7 +63,7 @@ type LegacyComposableControllerStateConstraint = { */ // TODO: Replace with `{ [name: string]: StateConstraint }` once BaseControllerV2 migrations are completed for all controllers. export type ComposableControllerStateConstraint = { - [name: string]: LegacyControllerStateConstraint; + [controllerName: string]: LegacyControllerStateConstraint; }; /** @@ -125,12 +149,12 @@ export type ComposableControllerMessenger< /** * Controller that composes multiple child controllers and maintains up-to-date composed state. * - * @template ComposableControllerState - A type object containing the names and state types of the child controllers. - * @template ChildControllers - A union type of the child controllers being used to instantiate the {@link ComposableController}. + * @template ComposableControllerState - A type object containing the names and state types of the child controllers. Any non-controllers with empty state should be omitted from this type argument. + * @template ChildControllers - A union type of the child controllers being used to instantiate the {@link ComposableController}. Non-controllers that are passed into the `controllers` constructor option at runtime should be included in this type argument. */ export class ComposableController< ComposableControllerState extends LegacyComposableControllerStateConstraint, - ChildControllers extends ControllerInstance, + ChildControllers extends NamedModule, > extends BaseController< typeof controllerName, ComposableControllerState, @@ -140,7 +164,7 @@ export class ComposableController< * Creates a ComposableController instance. * * @param options - Initial options used to configure this controller - * @param options.controllers - List of child controller instances to compose. + * @param options.controllers - List of child controller instances to compose. Any non-controllers that are included here will be excluded from the composed state object and from `stateChange` event subscription. * @param options.messenger - A restricted controller messenger. */ @@ -158,18 +182,27 @@ export class ComposableController< super({ name: controllerName, metadata: controllers.reduce>( - (metadata, controller) => ({ - ...metadata, - [controller.name]: isBaseController(controller) - ? controller.metadata - : { persist: true, anonymous: true }, - }), + (metadata, controller) => { + if (!isController(controller)) { + return metadata; + } + return Object.assign( + metadata, + isBaseController(controller) + ? { [controller.name]: controller.metadata } + : { [controller.name]: { persist: true, anonymous: true } }, + ); + }, {} as never, ), state: controllers.reduce( - (state, controller) => { - return { ...state, [controller.name]: controller.state }; - }, + (state, controller) => + Object.assign( + state, + isController(controller) + ? { [controller.name]: controller.state } + : {}, + ), {} as never, ), messenger, @@ -185,17 +218,13 @@ export class ComposableController< * * @param controller - Controller instance to update */ - #updateChildController(controller: ControllerInstance): void { - const { name } = controller; - if (!isBaseController(controller) && !isBaseControllerV1(controller)) { - // False negative. `name` is a string type. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - throw new Error(`${name} - ${INVALID_CONTROLLER_ERROR}`); + #updateChildController(controller: ChildControllers): void { + if (!isController(controller)) { + return; } + const { name } = controller; try { this.messagingSystem.subscribe( - // False negative. `name` is a string type. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions `${name}:stateChange`, (childState: LegacyControllerStateConstraint) => { this.update((state) => { @@ -203,13 +232,11 @@ export class ComposableController< }); }, ); - } catch (error: unknown) { - // False negative. `name` is a string type. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - console.error(`${name} - ${String(error)}`); - } + // Invalid/non-existent event names from V1 controllers and non-controllers are expected, and should be handled without blocking ComposableController instantiation in downstream clients. + // eslint-disable-next-line no-empty + } catch (error: unknown) {} if (isBaseControllerV1(controller)) { - controller.subscribe((childState: StateConstraintV1) => { + 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 index 8a0b5f5da2..2d80bdbef1 100644 --- a/packages/composable-controller/src/index.ts +++ b/packages/composable-controller/src/index.ts @@ -4,4 +4,7 @@ export type { ComposableControllerEvents, ComposableControllerMessenger, } from './ComposableController'; -export { ComposableController } from './ComposableController'; +export { + ComposableController, + STATELESS_NONCONTROLLER_NAMES, +} from './ComposableController';