Use DI for controller communication rather than context

We now setup inter-controller communication using dependency injection rather than the `context` property. Any dependency a controller has is passed in via a constructor parameter. This was done in preparation for migrating to BaseControllerV2. The style of dependency injection here matches the extension. Just as we do there, we inject state snapshots, means of subscribing to state, and individual methods rather than entire controllers. This helps to simplify tests and makes it easier to understand how controllers interact.
MetaMask · Mar 19, 2021 · c661433 · c661433
1 parent 10c9611
commit c661433
Show file tree

Hide file tree

Showing 23 changed files with 647 additions and 673 deletions.
diff --git a/README.md b/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.

diff --git a/src/BaseController.test.ts b/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();
-  });
 });
diff --git a/src/BaseController.ts b/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
    *

diff --git a/src/ComposableController.test.ts b/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(
+      (listener) => preferencesController.subscribe(listener),
+      (listener) => networkController.subscribe(listener),
+      assetContractController.getAssetName.bind(assetContractController),
+      assetContractController.getAssetSymbol.bind(assetContractController),
+      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(
+        (listener) => assetController.subscribe(listener),
+        (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(
+      (listener) => preferencesController.subscribe(listener),
+      (listener) => networkController.subscribe(listener),
+      assetContractController.getAssetName.bind(assetContractController),
+      assetContractController.getAssetSymbol.bind(assetContractController),
+      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(
+        (listener) => assetController.subscribe(listener),
+        (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', () => {

diff --git a/src/ComposableController.ts b/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;
   }