Mostly avoid asynchronous work in constructors

jest.config.js

@@ -16,14 +16,14 @@ module.exports = {
     '<rootDir>/**/src/**/*.ts',
     '!<rootDir>/**/src/**/*.test.ts',
   ],
-  coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'],
+  coverageReporters: ['html', 'json-summary', 'text'],
   coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'],
   coverageThreshold: {
     global: {
-      branches: 50.67,
-      functions: 50.63,
-      lines: 54.59,
-      statements: 54.76,
+      branches: 51.12,
+      functions: 48.24,
+      lines: 54.96,
+      statements: 55.12,
     },
   },
   projects: [

mocks/DuplexStream.ts

@@ -1,6 +1,6 @@
 import { Duplex } from 'stream';
 
-export default class DuplexStream extends Duplex {
+export class MockDuplexStream extends Duplex {
   constructor() {
     super({
       objectMode: true,

src/BaseProvider.ts

@@ -63,7 +63,7 @@ export interface BaseProviderState {
  * 3. Ensure that the provider's state is synchronized with the wallet.
  * 4. Ensure that notifications are received and emitted as appropriate.
  */
-export default abstract class BaseProvider extends SafeEventEmitter {
+export abstract class BaseProvider extends SafeEventEmitter {
   protected readonly _log: ConsoleLike;
 
   protected _state: BaseProviderState;
@@ -202,6 +202,8 @@ export default abstract class BaseProvider extends SafeEventEmitter {
   //====================
 
   /**
+   * **MUST** be called by child classes.
+   *
    * Sets initial state if provided and marks this provider as initialized.
    * Throws if called more than once.
    *

src/MetaMaskInpageProvider.test.ts

@@ -1,5 +1,5 @@
-import MockDuplexStream from '../mocks/DuplexStream';
-import MetaMaskInpageProvider from './MetaMaskInpageProvider';
+import { MockDuplexStream } from '../mocks/DuplexStream';
+import { MetaMaskInpageProvider } from './MetaMaskInpageProvider';
 import messages from './messages';
 
 describe('MetaMaskInpageProvider: RPC', () => {

src/MetaMaskInpageProvider.ts

@@ -1,7 +1,7 @@
 import type { Duplex } from 'stream';
 import type { JsonRpcRequest, JsonRpcResponse } from 'json-rpc-engine';
 import { ethErrors } from 'eth-rpc-errors';
-import sendSiteMetadata from './siteMetadata';
+import { sendSiteMetadata } from './siteMetadata';
 import messages from './messages';
 import {
   EMITTED_NOTIFICATIONS,
@@ -10,7 +10,10 @@ import {
   NOOP,
 } from './utils';
 import type { UnvalidatedJsonRpcRequest } from './BaseProvider';
-import StreamProvider, { StreamProviderOptions } from './StreamProvider';
+import {
+  AbstractStreamProvider,
+  StreamProviderOptions,
+} from './StreamProvider';
 
 export interface SendSyncJsonRpcRequest extends JsonRpcRequest<unknown> {
   method:
@@ -44,7 +47,7 @@ interface SentWarningsState {
   };
 }
 
-export default class MetaMaskInpageProvider extends StreamProvider {
+export class MetaMaskInpageProvider extends AbstractStreamProvider {
   protected _sentWarnings: SentWarningsState = {
     // methods
     enable: false,
@@ -100,6 +103,11 @@ export default class MetaMaskInpageProvider extends StreamProvider {
       rpcMiddleware: getDefaultExternalMiddleware(logger),
     });
 
+    // We shouldn't perform asynchronous work in the constructor, but at one
+    // point we started doing so, and changing this class isn't worth it at
+    // the time of writing.
+    this._initializeAsync();
+
     this.networkVersion = null;
     this.isMetaMask = true;
 

src/StreamProvider.test.ts

@@ -1,6 +1,6 @@
 import type { JsonRpcMiddleware } from 'json-rpc-engine';
-import MockDuplexStream from '../mocks/DuplexStream';
-import StreamProvider from './StreamProvider';
+import { MockDuplexStream } from '../mocks/DuplexStream';
+import { StreamProvider } from './StreamProvider';
 import messages from './messages';
 
 const MOCK_ERROR_MESSAGE = 'Did you specify a mock return value?';
@@ -21,8 +21,10 @@ describe('StreamProvider', () => {
       const networkVersion = '1';
       const isUnlocked = true;
 
+      const streamProvider = new StreamProvider(new MockDuplexStream());
+
       const requestMock = jest
-        .spyOn(StreamProvider.prototype, 'request')
+        .spyOn(streamProvider, 'request')
         .mockImplementationOnce(async () => {
           return {
             accounts,
@@ -32,16 +34,11 @@ describe('StreamProvider', () => {
           };
         });
 
-      const [streamProvider] = initializeProvider();
+      await streamProvider.initialize();
 
-      await new Promise<void>((resolve) => {
-        streamProvider.once('_initialized', () => {
-          expect(streamProvider.chainId).toBe(chainId);
-          expect(streamProvider.selectedAddress).toBe(accounts[0]);
-          expect(streamProvider.isConnected()).toBe(true);
-          resolve();
-        });
-      });
+      expect(streamProvider.chainId).toBe(chainId);
+      expect(streamProvider.selectedAddress).toBe(accounts[0]);
+      expect(streamProvider.isConnected()).toBe(true);
 
       expect(requestMock).toHaveBeenCalledTimes(1);
       expect(requestMock).toHaveBeenCalledWith({

src/StreamProvider.ts

@@ -7,7 +7,7 @@ import { createStreamMiddleware } from 'json-rpc-middleware-stream';
 import pump from 'pump';
 import messages from './messages';
 import { EMITTED_NOTIFICATIONS } from './utils';
-import BaseProvider, { BaseProviderOptions } from './BaseProvider';
+import { BaseProvider, BaseProviderOptions } from './BaseProvider';
 
 export interface StreamProviderOptions extends BaseProviderOptions {
   /**
@@ -23,10 +23,11 @@ export interface JsonRpcConnection {
 }
 
 /**
- * An EIP-1193 provider wired to a some duplex stream via a
- * `json-rpc-middleware-stream` JSON-RPC stream middleware.
+ * An abstract EIP-1193 provider wired to a some duplex stream via a
+ * `json-rpc-middleware-stream` JSON-RPC stream middleware. Implementers must
+ * directly call
  */
-export default class StreamProvider extends BaseProvider {
+export abstract class AbstractStreamProvider extends BaseProvider {
   protected _jsonRpcConnection: JsonRpcConnection;
 
   /**
@@ -77,9 +78,6 @@ export default class StreamProvider extends BaseProvider {
     // Wire up the JsonRpcEngine to the JSON-RPC connection stream
     this._rpcEngine.push(this._jsonRpcConnection.middleware);
 
-    // Set initial state
-    this._initializeAsync();
-
     // Handle JSON-RPC notifications
     this._jsonRpcConnection.events.on('notification', (payload) => {
       const { method, params } = payload;
@@ -107,11 +105,13 @@ export default class StreamProvider extends BaseProvider {
   //====================
 
   /**
-   * Constructor helper. Calls `metamask_getProviderState` and passes the result
-   * to {@link BaseProvider._initialize}. Logs an error if getting initial state
-   * fails.
+   * **MUST** be called by child classes.
+   *
+   * Calls `metamask_getProviderState` and passes the result to
+   * {@link BaseProvider._initialize}. Logs an error if getting initial state
+   * fails. Throws if called after initialization has completed.
    */
-  private async _initializeAsync() {
+  protected async _initializeAsync() {
     let initialState: Parameters<BaseProvider['_initialize']>[0];
 
     try {
@@ -147,3 +147,22 @@ export default class StreamProvider extends BaseProvider {
     this._handleDisconnect(false, error ? error.message : undefined);
   }
 }
+
+/**
+ * An EIP-1193 provider wired to a some duplex stream via a
+ * `json-rpc-middleware-stream` JSON-RPC stream middleware. Consumers must
+ * call {@link StreamProvider.initialize} after instantiation to complete
+ * initialization.
+ */
+export class StreamProvider extends AbstractStreamProvider {
+  /**
+   * **MUST** be called after instantiation to complete initialization.
+   *
+   * Calls `metamask_getProviderState` and passes the result to
+   * {@link BaseProvider._initialize}. Logs an error if getting initial state
+   * fails. Throws if called after initialization has completed.
+   */
+  async initialize() {
+    return this._initializeAsync();
+  }
+}

src/extension-provider/createExternalExtensionProvider.test.ts

@@ -1,5 +1,5 @@
-import StreamProvider from '../StreamProvider';
-import createExternalExtensionProvider from './createExternalExtensionProvider';
+import { StreamProvider } from '../StreamProvider';
+import { createExternalExtensionProvider } from './createExternalExtensionProvider';
 
 describe('createExternalExtensionProvider', () => {
   beforeEach(() => {

src/extension-provider/createExternalExtensionProvider.ts

@@ -1,13 +1,13 @@
 import PortStream from 'extension-port-stream';
 import { detect } from 'detect-browser';
 import { Runtime } from 'webextension-polyfill-ts';
-import StreamProvider from '../StreamProvider';
+import { StreamProvider } from '../StreamProvider';
 import { getDefaultExternalMiddleware } from '../utils';
 import config from './external-extension-config.json';
 
 const browser = detect();
 
-export default function createMetaMaskExternalExtensionProvider() {
+export function createExternalExtensionProvider() {
   let provider;
 
   try {
@@ -21,9 +21,14 @@ export default function createMetaMaskExternalExtensionProvider() {
       logger: console,
       rpcMiddleware: getDefaultExternalMiddleware(console),
     });
-  } catch (e) {
-    console.dir(`Metamask connect error `, e);
-    throw e;
+
+    // This is asynchronous but merely logs an error and does not throw upon
+    // failure. Previously this just happened as a side-effect in the
+    // constructor.
+    provider.initialize();
+  } catch (error) {
+    console.dir(`MetaMask connect error.`, error);
+    throw error;
   }
   return provider;
 }

src/index.ts

@@ -1,12 +1,12 @@
-import BaseProvider from './BaseProvider';
-import createExternalExtensionProvider from './extension-provider/createExternalExtensionProvider';
+import { BaseProvider } from './BaseProvider';
+import { createExternalExtensionProvider } from './extension-provider/createExternalExtensionProvider';
 import {
   initializeProvider,
   setGlobalProvider,
 } from './initializeInpageProvider';
-import MetaMaskInpageProvider from './MetaMaskInpageProvider';
-import shimWeb3 from './shimWeb3';
-import StreamProvider from './StreamProvider';
+import { MetaMaskInpageProvider } from './MetaMaskInpageProvider';
+import { shimWeb3 } from './shimWeb3';
+import { StreamProvider } from './StreamProvider';
 
 export {
   BaseProvider,

src/initializeInpageProvider.ts

@@ -1,8 +1,9 @@
 import { Duplex } from 'stream';
-import MetaMaskInpageProvider, {
+import {
+  MetaMaskInpageProvider,
   MetaMaskInpageProviderOptions,
 } from './MetaMaskInpageProvider';
-import shimWeb3 from './shimWeb3';
+import { shimWeb3 } from './shimWeb3';
 
 interface InitializeProviderOptions extends MetaMaskInpageProviderOptions {
   /**

src/shimWeb3.ts

@@ -1,4 +1,4 @@
-import MetaMaskInpageProvider from './MetaMaskInpageProvider';
+import { MetaMaskInpageProvider } from './MetaMaskInpageProvider';
 import { ConsoleLike } from './utils';
 
 /**
@@ -8,7 +8,7 @@ import { ConsoleLike } from './utils';
  * @param provider - The provider to set as window.web3.currentProvider.
  * @param log - The logging API to use.
  */
-export default function shimWeb3(
+export function shimWeb3(
   provider: MetaMaskInpageProvider,
   log: ConsoleLike = console,
 ): void {

src/siteMetadata.ts

@@ -9,7 +9,7 @@ import { ConsoleLike, NOOP } from './utils';
  * @param engine - The JSON RPC Engine to send metadata over.
  * @param log - The logging API to use.
  */
-export default async function sendSiteMetadata(
+export async function sendSiteMetadata(
   engine: JsonRpcEngine,
   log: ConsoleLike,
 ): Promise<void> {