Use DI for controller communication rather than context

README.md

@@ -245,8 +245,6 @@ console.log(datamodel.state); // {NetworkController: {...}, TokenRatesController
 console.log(datamodel.flatState); // {infura: {...}, contractExchangeRates: [...]}
 ```
 
-**Advanced Note:** The ComposableController builds a map of all child controllers keyed by controller name. This object is cached as a `context` instance variable on both the ComposableController itself as well as all child controllers. This means that child controllers can call methods on other sibling controllers through the `context` variable, e.g. `this.context.SomeController.someMethod()`.
-
 ## Linking during development
 
 Linking `@metamask/controllers` into other projects involves a special NPM command to ensure that dependencies are not duplicated. This is because `@metamask/controllers` ships modules that are transpiled but not bundled, and [NPM does not deduplicate](https://github.com/npm/npm/issues/7742) linked dependency trees.

src/BaseController.test.ts

@@ -1,13 +1,10 @@
 import { stub } from 'sinon';
 import BaseController, { BaseConfig, BaseState } from './BaseController';
-import ComposableController from './ComposableController';
 
 const STATE = { name: 'foo' };
 const CONFIG = { disabled: true };
 
 class TestController extends BaseController<BaseConfig, BaseState> {
-  requiredControllers = ['Foo'];
-
   constructor(config?: BaseConfig, state?: BaseState) {
     super(config, state);
     this.initialize();
@@ -68,11 +65,4 @@ describe('BaseController', () => {
     controller.notify();
     expect(listener.called).toBe(false);
   });
-
-  it('should throw if siblings are missing dependencies', () => {
-    const controller = new TestController();
-    expect(() => {
-      new ComposableController([controller]);
-    }).toThrow('BaseController must be composed with Foo.');
-  });
 });

src/BaseController.ts

@@ -1,5 +1,3 @@
-import { ChildControllerContext } from './ComposableController';
-
 /**
  * State change callbacks
  */
@@ -31,13 +29,6 @@ export interface BaseState {
  * Controller class that provides configuration, state management, and subscriptions
  */
 export class BaseController<C extends BaseConfig, S extends BaseState> {
-  /**
-   * Map of all sibling child controllers keyed by name if this
-   * controller is composed using a ComposableController, allowing
-   * any API on any sibling controller to be accessed
-   */
-  context: ChildControllerContext = {};
-
   /**
    * Default options used to configure this controller
    */
@@ -58,11 +49,6 @@ export class BaseController<C extends BaseConfig, S extends BaseState> {
    */
   name = 'BaseController';
 
-  /**
-   * List of required sibling controllers this controller needs to function
-   */
-  requiredControllers: string[] = [];
-
   private readonly initialConfig: C;
 
   private readonly initialState: S;
@@ -158,18 +144,6 @@ export class BaseController<C extends BaseConfig, S extends BaseState> {
     });
   }
 
-  /**
-   * Extension point called if and when this controller is composed
-   * with other controllers using a ComposableController
-   */
-  onComposed() {
-    this.requiredControllers.forEach((name) => {
-      if (!this.context[name]) {
-        throw new Error(`${this.name} must be composed with ${name}.`);
-      }
-    });
-  }
-
   /**
    * Adds new listener to be notified of state changes
    *

src/ComposableController.test.ts

@@ -11,15 +11,29 @@ import CurrencyRateController from './assets/CurrencyRateController';
 
 describe('ComposableController', () => {
   it('should compose controller state', () => {
+    const preferencesController = new PreferencesController();
+    const networkController = new NetworkController();
+    const assetContractController = new AssetsContractController();
+    const assetController = new AssetsController({
+      onPreferencesStateChange: (listener) => preferencesController.subscribe(listener),
+      onNetworkStateChange: (listener) => networkController.subscribe(listener),
+      getAssetName: assetContractController.getAssetName.bind(assetContractController),
+      getAssetSymbol: assetContractController.getAssetSymbol.bind(assetContractController),
+      getCollectibleTokenURI: assetContractController.getCollectibleTokenURI.bind(assetContractController),
+    });
+    const currencyRateController = new CurrencyRateController();
     const controller = new ComposableController([
       new AddressBookController(),
-      new AssetsController(),
-      new AssetsContractController(),
+      assetController,
+      assetContractController,
       new EnsController(),
-      new CurrencyRateController(),
-      new NetworkController(),
-      new PreferencesController(),
-      new TokenRatesController(),
+      currencyRateController,
+      networkController,
+      preferencesController,
+      new TokenRatesController({
+        onAssetsStateChange: (listener) => assetController.subscribe(listener),
+        onCurrencyRateStateChange: (listener) => currencyRateController.subscribe(listener),
+      }),
     ]);
     expect(controller.state).toEqual({
       AddressBookController: { addressBook: {} },
@@ -62,15 +76,29 @@ describe('ComposableController', () => {
   });
 
   it('should compose flat controller state', () => {
+    const preferencesController = new PreferencesController();
+    const networkController = new NetworkController();
+    const assetContractController = new AssetsContractController();
+    const assetController = new AssetsController({
+      onPreferencesStateChange: (listener) => preferencesController.subscribe(listener),
+      onNetworkStateChange: (listener) => networkController.subscribe(listener),
+      getAssetName: assetContractController.getAssetName.bind(assetContractController),
+      getAssetSymbol: assetContractController.getAssetSymbol.bind(assetContractController),
+      getCollectibleTokenURI: assetContractController.getCollectibleTokenURI.bind(assetContractController),
+    });
+    const currencyRateController = new CurrencyRateController();
     const controller = new ComposableController([
       new AddressBookController(),
-      new AssetsController(),
-      new AssetsContractController(),
+      assetController,
+      assetContractController,
       new EnsController(),
-      new CurrencyRateController(),
-      new NetworkController(),
-      new PreferencesController(),
-      new TokenRatesController(),
+      currencyRateController,
+      networkController,
+      preferencesController,
+      new TokenRatesController({
+        onAssetsStateChange: (listener) => assetController.subscribe(listener),
+        onCurrencyRateStateChange: (listener) => currencyRateController.subscribe(listener),
+      }),
     ]);
     expect(controller.flatState).toEqual({
       addressBook: {},
@@ -101,85 +129,22 @@ describe('ComposableController', () => {
     });
   });
 
-  it('should expose sibling context', () => {
-    const controller = new ComposableController([
-      new AddressBookController(),
-      new AssetsController(),
-      new AssetsContractController(),
-      new CurrencyRateController(),
-      new EnsController(),
-      new NetworkController(),
-      new PreferencesController(),
-      new TokenRatesController(),
-    ]);
-    const addressContext = controller.context.TokenRatesController.context
-      .AddressBookController as AddressBookController;
-    expect(addressContext).toBeDefined();
-    addressContext.set('0x32Be343B94f860124dC4fEe278FDCBD38C102D88', 'foo');
-    expect(controller.flatState).toEqual({
+  it('should set initial state', () => {
+    const state = {
       addressBook: {
-        1: {
-          '0x32Be343B94f860124dC4fEe278FDCBD38C102D88': {
-            address: '0x32Be343B94f860124dC4fEe278FDCBD38C102D88',
+        '0x1': {
+          '0x1234': {
+            address: 'bar',
             chainId: '1',
             isEns: false,
             memo: '',
             name: 'foo',
           },
         },
       },
-      allCollectibleContracts: {},
-      allCollectibles: {},
-      allTokens: {},
-      collectibleContracts: [],
-      collectibles: [],
-      contractExchangeRates: {},
-      conversionDate: 0,
-      conversionRate: 0,
-      currentCurrency: 'usd',
-      ensEntries: {},
-      featureFlags: {},
-      frequentRpcList: [],
-      identities: {},
-      ignoredCollectibles: [],
-      ignoredTokens: [],
-      ipfsGateway: 'https://ipfs.io/ipfs/',
-      lostIdentities: {},
-      nativeCurrency: 'ETH',
-      network: 'loading',
-      provider: { type: 'mainnet', chainId: NetworksChainId.mainnet },
-      selectedAddress: '',
-      suggestedAssets: [],
-      tokens: [],
-      usdConversionRate: 0,
-    });
-  });
-
-  it('should get and set new stores', () => {
-    const controller = new ComposableController();
-    const addressBook = new AddressBookController();
-    controller.controllers = [addressBook];
-    expect(controller.controllers).toEqual([addressBook]);
-  });
-
-  it('should set initial state', () => {
-    const state = {
-      AddressBookController: {
-        addressBook: [
-          {
-            1: {
-              address: 'bar',
-              chainId: '1',
-              isEns: false,
-              memo: '',
-              name: 'foo',
-            },
-          },
-        ],
-      },
     };
-    const controller = new ComposableController([new AddressBookController()], state);
-    expect(controller.state).toEqual(state);
+    const controller = new ComposableController([new AddressBookController(undefined, state)]);
+    expect(controller.state).toEqual({ AddressBookController: state });
   });
 
   it('should notify listeners of nested state change', () => {

src/ComposableController.ts

@@ -1,12 +1,5 @@
 import BaseController from './BaseController';
 
-/**
- * Child controller instances keyed by controller name
- */
-export interface ChildControllerContext {
-  [key: string]: BaseController<any, any>;
-}
-
 /**
  * List of child controller instances
  */
@@ -15,15 +8,8 @@ export type ControllerList = BaseController<any, any>[];
 /**
  * Controller that can be used to compose multiple controllers together
  */
-export class ComposableController extends BaseController<any, any> {
-  private cachedState: any;
-
-  private internalControllers: ControllerList = [];
-
-  /**
-   * Map of stores to compose together
-   */
-  context: ChildControllerContext = {};
+export class ComposableController extends BaseController<never, any> {
+  private controllers: ControllerList = [];
 
   /**
    * Name of this controller used during composition
@@ -36,45 +22,22 @@ export class ComposableController extends BaseController<any, any> {
    * @param controllers - Map of names to controller instances
    * @param initialState - Initial state keyed by child controller name
    */
-  constructor(controllers: ControllerList = [], initialState?: any) {
-    super();
+  constructor(controllers: ControllerList) {
+    super(
+      undefined,
+      controllers.reduce((state, controller) => {
+        state[controller.name] = controller.state;
+        return state;
+      }, {} as any),
+    );
     this.initialize();
-    this.cachedState = initialState;
     this.controllers = controllers;
-    this.cachedState = undefined;
-  }
-
-  /**
-   * Get current list of child composed store instances
-   *
-   * @returns - List of names to controller instances
-   */
-  get controllers() {
-    return this.internalControllers;
-  }
-
-  /**
-   * Set new list of controller instances
-   *
-   * @param controllers - List of names to controller instsances
-   */
-  set controllers(controllers: ControllerList) {
-    this.internalControllers = controllers;
-    const initialState: any = {};
-    controllers.forEach((controller) => {
+    this.controllers.forEach((controller) => {
       const { name } = controller;
-      this.context[name] = controller;
-      controller.context = this.context;
-      this.cachedState?.[name] && controller.update(this.cachedState[name]);
-      initialState[name] = controller.state;
       controller.subscribe((state) => {
         this.update({ [name]: state });
       });
     });
-    controllers.forEach((controller) => {
-      controller.onComposed();
-    });
-    this.update(initialState, true);
   }
 
   /**
@@ -86,8 +49,8 @@ export class ComposableController extends BaseController<any, any> {
    */
   get flatState() {
     let flatState = {};
-    for (const name in this.context) {
-      flatState = { ...flatState, ...this.context[name].state };
+    for (const controller of this.controllers) {
+      flatState = { ...flatState, ...controller.state };
     }
     return flatState;
   }