Remove ConnectionPool (#5469)

Author: schmidt-sebastian

common/api-review/storage.api.md

@@ -58,7 +58,7 @@ export class _FirebaseStorageImpl implements FirebaseStorage {
     constructor(
     app: FirebaseApp, _authProvider: Provider<FirebaseAuthInternalName>,
     _appCheckProvider: Provider<AppCheckInternalComponentName>,
-    _pool: ConnectionPool, _url?: string | undefined, _firebaseVersion?: string | undefined);
+    _url?: string | undefined, _firebaseVersion?: string | undefined);
     readonly app: FirebaseApp;
     // (undocumented)
     readonly _appCheckProvider: Provider<AppCheckInternalComponentName>;
@@ -78,23 +78,20 @@ export class _FirebaseStorageImpl implements FirebaseStorage {
     get host(): string;
     set host(host: string);
     // Warning: (ae-forgotten-export) The symbol "RequestInfo" needs to be exported by the entry point index.d.ts
+    // Warning: (ae-forgotten-export) The symbol "Connection" needs to be exported by the entry point index.d.ts
     // Warning: (ae-forgotten-export) The symbol "Request" needs to be exported by the entry point index.d.ts
     //
     // (undocumented)
-    _makeRequest<T>(requestInfo: RequestInfo_2<T>, authToken: string | null, appCheckToken: string | null): Request_2<T>;
+    _makeRequest<T>(requestInfo: RequestInfo_2<T>, requestFactory: () => Connection, authToken: string | null, appCheckToken: string | null): Request_2<T>;
     // (undocumented)
-    makeRequestWithTokens<T>(requestInfo: RequestInfo_2<T>): Promise<Request_2<T>>;
+    makeRequestWithTokens<T>(requestInfo: RequestInfo_2<T>, requestFactory: () => Connection): Promise<T>;
     _makeStorageReference(loc: _Location): _Reference;
     get maxOperationRetryTime(): number;
     set maxOperationRetryTime(time: number);
     get maxUploadRetryTime(): number;
     set maxUploadRetryTime(time: number);
     // (undocumented)
     _overrideAuthToken?: string;
-    // Warning: (ae-forgotten-export) The symbol "ConnectionPool" needs to be exported by the entry point index.d.ts
-    //
-    // (undocumented)
-    readonly _pool: ConnectionPool;
     // (undocumented)
     _protocol: string;
     // (undocumented)

packages/storage-compat/src/task.ts

@@ -17,7 +17,7 @@
 
 import {
   UploadTask,
-  FirebaseStorageError,
+  StorageError,
   UploadTaskSnapshot,
   TaskEvent,
   StorageObserver
@@ -48,7 +48,7 @@ export class UploadTaskCompat implements types.UploadTask, Compat<UploadTask> {
 
   then(
     onFulfilled?: ((a: UploadTaskSnapshotCompat) => unknown) | null,
-    onRejected?: ((a: FirebaseStorageError) => unknown) | null
+    onRejected?: ((a: StorageError) => unknown) | null
   ): Promise<unknown> {
     return this._delegate.then(snapshot => {
       if (onFulfilled) {
@@ -65,7 +65,7 @@ export class UploadTaskCompat implements types.UploadTask, Compat<UploadTask> {
       | types.StorageObserver<UploadTaskSnapshotCompat>
       | null
       | ((a: UploadTaskSnapshotCompat) => unknown),
-    error?: (error: FirebaseStorageError) => void | null,
+    error?: (error: StorageError) => void | null,
     completed?: () => void | null
   ): Unsubscribe | Subscribe<UploadTaskSnapshotCompat> {
     let wrappedNextOrObserver:
@@ -123,7 +123,7 @@ export type NextFn<T> = (value: T) => void;
  * A function that is called with a `FirebaseStorageError`
  * if the event stream ends due to an error.
  */
-export type ErrorFn = (error: FirebaseStorageError) => void;
+export type ErrorFn = (error: StorageError) => void;
 
 /**
  * A function that is called if the event stream ends normally.

packages/storage-compat/test/unit/reference.test.ts

@@ -162,15 +162,15 @@ describe('Firebase Storage > Reference', () => {
 
   describe('Argument verification', () => {
     describe('list', () => {
-      it('throws on invalid maxResults', async () => {
+      it('throws on invalid maxResults', () => {
         const child = service.refFromURL('gs://test-bucket/hello');
-        await expect(child.list({ maxResults: 0 })).to.be.rejectedWith(
+        expect(() => child.list({ maxResults: 0 })).to.throw(
           'storage/invalid-argument'
         );
-        await expect(child.list({ maxResults: -4 })).to.be.rejectedWith(
+        expect(() => child.list({ maxResults: -4 })).to.throw(
           'storage/invalid-argument'
         );
-        await expect(child.list({ maxResults: 1001 })).to.be.rejectedWith(
+        expect(() => child.list({ maxResults: 1001 })).to.throw(
           'storage/invalid-argument'
         );
       });
@@ -195,18 +195,18 @@ describe('Firebase Storage > Reference', () => {
     it('delete throws', () => {
       expect(() => root.delete()).to.throw('storage/invalid-root-operation');
     });
-    it('getMetadata throws', async () => {
-      await expect(root.getMetadata()).to.be.rejectedWith(
+    it('getMetadata throws', () => {
+      expect(() => root.getMetadata()).to.throw(
         'storage/invalid-root-operation'
       );
     });
-    it('updateMetadata throws', async () => {
-      await expect(root.updateMetadata({})).to.be.rejectedWith(
+    it('updateMetadata throws', () => {
+      expect(() => root.updateMetadata({})).to.throw(
         'storage/invalid-root-operation'
       );
     });
     it('getDownloadURL throws', async () => {
-      await expect(root.getDownloadURL()).to.be.rejectedWith(
+      expect(() => root.getDownloadURL()).to.throw(
         'storage/invalid-root-operation'
       );
     });

packages/storage-compat/test/unit/service.test.ts

@@ -19,11 +19,7 @@ import { expect } from 'chai';
 import { stub } from 'sinon';
 import * as modularStorage from '@firebase/storage';
 import { makeTestCompatStorage, fakeApp, fakeStorage } from '../utils';
-import {
-  FirebaseStorage,
-  getStorage,
-  FirebaseStorageError
-} from '@firebase/storage';
+import { FirebaseStorage, getStorage, StorageError } from '@firebase/storage';
 import firebase from '@firebase/app-compat';
 import { StorageServiceCompat } from '../../src/service';
 import { FirebaseApp } from '@firebase/app-types';
@@ -215,10 +211,8 @@ GOOG4-RSA-SHA256`
         ref.put(new Uint8Array([97])).on(
           'state_changed',
           null,
-          (err: FirebaseStorageError | Error) => {
-            expect((err as FirebaseStorageError).code).to.equal(
-              'storage/app-deleted'
-            );
+          (err: StorageError | Error) => {
+            expect((err as StorageError).code).to.equal('storage/app-deleted');
             resolve();
           },
           () => {

packages/storage/src/implementation/connectionPool.ts

@@ -32,7 +32,6 @@ import { RequestHandler, RequestInfo } from './requestinfo';
 import { isJustDef } from './type';
 import { makeQueryString } from './url';
 import { Headers, Connection, ErrorCode } from './connection';
-import { ConnectionPool } from './connectionPool';
 
 export interface Request<T> {
   getPromise(): Promise<T>;
@@ -68,7 +67,7 @@ class NetworkRequest<T> implements Request<T> {
     private errorCallback_: RequestHandler<StorageError, StorageError> | null,
     private timeout_: number,
     private progressCallback_: ((p1: number, p2: number) => void) | null,
-    private pool_: ConnectionPool
+    private connectionFactory_: () => Connection
   ) {
     this.promise_ = new Promise((resolve, reject) => {
       this.resolve_ = resolve as (value?: T | PromiseLike<T>) => void;
@@ -89,7 +88,7 @@ class NetworkRequest<T> implements Request<T> {
         backoffCallback(false, new RequestEndStatus(false, null, true));
         return;
       }
-      const connection = this.pool_.createConnection();
+      const connection = this.connectionFactory_();
       this.pendingConnection_ = connection;
 
       const progressListener: (progressEvent: ProgressEvent) => void =
@@ -272,7 +271,7 @@ export function makeRequest<T>(
   appId: string | null,
   authToken: string | null,
   appCheckToken: string | null,
-  pool: ConnectionPool,
+  requestFactory: () => Connection,
   firebaseVersion?: string
 ): Request<T> {
   const queryPart = makeQueryString(requestInfo.urlParams);
@@ -293,6 +292,6 @@ export function makeRequest<T>(
     requestInfo.errorHandler,
     requestInfo.timeout,
     requestInfo.progressCallback,
-    pool
+    requestFactory
   );
 }

packages/storage/src/implementation/request.ts

@@ -40,7 +40,6 @@ import { name, version } from '../package.json';
 
 import { FirebaseStorage } from './public-types';
 import { STORAGE_TYPE } from './constants';
-import { ConnectionPool } from './implementation/connectionPool';
 
 export * from './api';
 export * from './api.node';
@@ -57,7 +56,6 @@ function factory(
     app,
     authProvider,
     appCheckProvider,
-    new ConnectionPool(),
     url,
     SDK_VERSION
   );

packages/storage/src/implementation/requestmaker.ts

@@ -27,7 +27,6 @@ import {
   SDK_VERSION
 } from '@firebase/app';
 
-import { ConnectionPool } from '../src/implementation/connectionPool';
 import { FirebaseStorageImpl } from '../src/service';
 import {
   Component,
@@ -56,7 +55,6 @@ function factory(
     app,
     authProvider,
     appCheckProvider,
-    new ConnectionPool(),
     url,
     SDK_VERSION
   );

packages/storage/src/index.node.ts

@@ -22,6 +22,9 @@ import {
 } from '../../implementation/connection';
 import { internalError } from '../../implementation/error';
 
+/** An override for the text-based Connection. Used in tests. */
+let connectionFactoryOverride: (() => Connection) | null = null;
+
 /**
  * Network layer for browsers. We use this instead of goog.net.XhrIo because
  * goog.net.XhrIo is hyuuuuge and doesn't work in React Native on Android.
@@ -124,5 +127,11 @@ export class XhrConnection implements Connection {
 }
 
 export function newConnection(): Connection {
-  return new XhrConnection();
+  return connectionFactoryOverride
+    ? connectionFactoryOverride()
+    : new XhrConnection();
+}
+
+export function injectTestConnection(factory: (() => Connection) | null): void {
+  connectionFactoryOverride = factory;
 }

packages/storage/src/index.ts

@@ -15,9 +15,17 @@
  * limitations under the License.
  */
 import { Connection } from '../implementation/connection';
-import { newConnection as nodeNewConnection } from './node/connection';
+import {
+  newConnection as nodeNewConnection,
+  injectTestConnection as nodeInjectTestConnection
+} from './node/connection';
 
 export function newConnection(): Connection {
   // This file is only used under ts-node.
   return nodeNewConnection();
 }
+
+export function injectTestConnection(factory: (() => Connection) | null): void {
+  // This file is only used under ts-node.
+  nodeInjectTestConnection(factory);
+}

packages/storage/src/platform/browser/connection.ts

@@ -19,6 +19,9 @@ import { ErrorCode, Connection } from '../../implementation/connection';
 import { internalError } from '../../implementation/error';
 import nodeFetch, { FetchError } from 'node-fetch';
 
+/** An override for the text-based Connection. Used in tests. */
+let connectionFactoryOverride: (() => Connection) | null = null;
+
 /**
  * Network layer that works in Node.
  *
@@ -118,5 +121,11 @@ export class FetchConnection implements Connection {
 }
 
 export function newConnection(): Connection {
-  return new FetchConnection();
+  return connectionFactoryOverride
+    ? connectionFactoryOverride()
+    : new FetchConnection();
+}
+
+export function injectTestConnection(factory: (() => Connection) | null): void {
+  connectionFactoryOverride = factory;
 }