Do not throw from connection.send in Node.js (#5578)

* do not throw from connection.send

* Create curvy-peaches-deliver.md

* add tests

* formatting

Author: Feiyang1

.changeset/curvy-peaches-deliver.md

@@ -0,0 +1,5 @@
+---
+"@firebase/storage": patch
+---
+
+Do not throw from `connection.send` to enable retries in Node.js.

packages/storage/src/implementation/connection.ts

@@ -23,6 +23,10 @@ export type Headers = Record<string, string>;
  * goog.net.XhrIo-like interface.
  */
 export interface Connection {
+  /**
+   * This method should never reject. In case of encountering an error, set an error code internally which can be accessed
+   * by calling getErrorCode() by callers. 
+   */
   send(
     url: string,
     method: string,

packages/storage/src/implementation/request.ts

@@ -106,6 +106,7 @@ class NetworkRequest<T> implements Request<T> {
         connection.addUploadProgressListener(progressListener);
       }
 
+      // connection.send() never rejects, so we don't need to have a error handler or use catch on the returned promise.
       // eslint-disable-next-line @typescript-eslint/no-floating-promises
       connection
         .send(this.url_, this.method_, this.body_, this.headers_)

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

@@ -17,10 +17,7 @@
 
 import { ErrorCode, Connection } from '../../implementation/connection';
 import { internalError } from '../../implementation/error';
-import nodeFetch from 'node-fetch';
-
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-const fetch: typeof window.fetch = nodeFetch as any;
+import nodeFetch, { FetchError } from 'node-fetch';
 
 /**
  * Network layer that works in Node.
@@ -34,6 +31,8 @@ export class FetchConnection implements Connection {
   private body_: string | undefined;
   private headers_: Headers | undefined;
   private sent_: boolean = false;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  private fetch_: typeof window.fetch = nodeFetch as any;
 
   constructor() {
     this.errorCode_ = ErrorCode.NO_ERROR;
@@ -50,18 +49,21 @@ export class FetchConnection implements Connection {
     }
     this.sent_ = true;
 
-    return fetch(url, {
+    return this.fetch_(url, {
       method,
       headers: headers || {},
       body
     })
       .then(resp => {
         this.headers_ = resp.headers;
         this.statusCode_ = resp.status;
-        return resp.text();
-      })
-      .then(body => {
-        this.body_ = body;
+        return resp.text().then(body => {
+          this.body_ = body;
+        });
+      }, (_err: FetchError) => {
+        this.errorCode_ = ErrorCode.NETWORK_ERROR;
+        // emulate XHR which sets status to 0 when encountering a network error
+        this.statusCode_ = 0;
       });
   }
 

packages/storage/test/browser/connection.test.ts

@@ -0,0 +1,17 @@
+import { expect } from "chai";
+import { SinonFakeXMLHttpRequest, useFakeXMLHttpRequest } from "sinon";
+import { ErrorCode } from "../../src/implementation/connection";
+import { XhrConnection } from "../../src/platform/browser/connection";
+
+describe('Connections', () => {
+    it('XhrConnection.send() should not reject on network errors', async () => {
+        const fakeXHR = useFakeXMLHttpRequest();
+        const connection = new XhrConnection();
+        const sendPromise = connection.send('testurl', 'GET');
+        // simulate a network error
+        ((connection as any).xhr_ as SinonFakeXMLHttpRequest).error();
+        await sendPromise;
+        expect(connection.getErrorCode()).to.equal(ErrorCode.NETWORK_ERROR);
+        fakeXHR.restore();
+    });
+});

packages/storage/test/unit/connection.test.ts

@@ -0,0 +1,15 @@
+import { stub } from 'sinon';
+import { expect } from 'chai';
+import { ErrorCode } from '../../src/implementation/connection';
+import { FetchConnection } from '../../src/platform/node/connection';
+
+describe('Connections', () => {
+    it('FetchConnection.send() should not reject on network errors', async () => {
+        const connection = new FetchConnection();
+
+        // need the casting here because fetch_ is a private member
+        stub(connection as any, 'fetch_').rejects();
+        await connection.send('testurl', 'GET');
+        expect(connection.getErrorCode()).to.equal(ErrorCode.NETWORK_ERROR);
+    });
+});