Merge pull request #8328 from Expensify/marcaaron-networkCleanup3

src/libs/API.js

@@ -9,7 +9,6 @@ import requireParameters from './requireParameters';
 import Log from './Log';
 import * as Network from './Network';
 import updateSessionAuthTokens from './actions/Session/updateSessionAuthTokens';
-import setSessionLoadingAndError from './actions/Session/setSessionLoadingAndError';
 import * as NetworkStore from './Network/NetworkStore';
 import enhanceParameters from './Network/enhanceParameters';
 import * as NetworkEvents from './Network/NetworkEvents';
@@ -48,21 +47,10 @@ function handleExpiredAuthToken(originalCommand, originalParameters, originalTyp
         ));
 }
 
+// We set the logger for Network here so that we can avoid a circular dependency
 NetworkEvents.registerLogHandler(() => Log);
 
-NetworkEvents.onRequestMade((queuedRequest, finalParameters) => {
-    if (queuedRequest.command === 'Log') {
-        return;
-    }
-
-    Log.info('Making API request', false, {
-        command: queuedRequest.command,
-        type: queuedRequest.type,
-        shouldUseSecure: queuedRequest.shouldUseSecure,
-        rvl: finalParameters.returnValueList,
-    });
-});
-
+// Handle response event sent by the Network
 NetworkEvents.onResponse((queuedRequest, response) => {
     if (queuedRequest.command !== 'Log') {
         Log.info('Finished API request', false, {
@@ -100,6 +88,7 @@ NetworkEvents.onResponse((queuedRequest, response) => {
         return;
     }
 
+    // Check to see if queuedRequest has a resolve method as this could be a persisted request which had it's promise handling logic stripped
     if (!queuedRequest.resolve) {
         return;
     }
@@ -108,28 +97,6 @@ NetworkEvents.onResponse((queuedRequest, response) => {
     queuedRequest.resolve(response);
 });
 
-NetworkEvents.onError((queuedRequest, error) => {
-    if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
-        Log.info('[API] request canceled', false, queuedRequest);
-        return;
-    }
-    if (queuedRequest.command !== 'Log') {
-        Log.hmmm('[API] Handled error when making request', error);
-    } else {
-        console.debug('[API] There was an error in the Log API command, unable to log to server!', error);
-    }
-
-    // Set an error state and signify we are done loading
-    setSessionLoadingAndError(false, 'Cannot connect to server');
-
-    // Reject the queued request with an API offline error so that the original caller can handle it
-    // Note: We are checking for the reject method as this could be a persisted request which had it's promise handling logic stripped
-    // from it when persisted to storage
-    if (queuedRequest.reject) {
-        queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE));
-    }
-});
-
 /**
  * @param {Object} parameters
  * @param {Boolean} [parameters.useExpensifyLogin]

src/libs/Network/NetworkEvents.js

@@ -1,11 +1,14 @@
 import CONST from '../../CONST';
 import createCallback from '../createCallback';
 
+/**
+ * This file helps bridge the Network layer with other parts of the app like API, NetworkConnection, PersistedRequestsQueue etc.
+ * It helps avoid circular dependencies and by setting up event triggers and subscribers.
+ */
+
 const [getLogger, registerLogHandler] = createCallback();
 const [triggerConnectivityResumed, onConnectivityResumed] = createCallback();
-const [triggerRequestMade, onRequestMade] = createCallback();
 const [triggerResponse, onResponse] = createCallback();
-const [triggerError, onError] = createCallback();
 const [triggerRecheckNeeded, onRecheckNeeded] = createCallback();
 
 /**
@@ -22,12 +25,8 @@ export {
     getLogger,
     triggerConnectivityResumed,
     onConnectivityResumed,
-    triggerRequestMade,
-    onRequestMade,
     onResponse,
     triggerResponse,
-    onError,
-    triggerError,
     triggerRecheckNeeded,
     onRecheckNeeded,
     startRecheckTimeoutTimer,

src/libs/Network/PersistedRequestsQueue.js

@@ -27,33 +27,11 @@ function process() {
     }
 
     const tasks = _.map(persistedRequests, request => processRequest(request)
-        .then((response) => {
-            if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) {
-                NetworkEvents.getLogger().info('Persisted optimistic request needs authentication');
-            } else {
-                NetworkEvents.getLogger().info('Persisted optimistic request returned a valid jsonCode. Not retrying.');
-            }
-            NetworkEvents.triggerResponse(request, response);
-            PersistedRequests.remove(request);
-        })
         .catch((error) => {
-            // Make this request if we are catching a known network error like "Failed to fetch" and have retries left
-            if (error.message === CONST.ERROR.FAILED_TO_FETCH) {
-                const retryCount = PersistedRequests.incrementRetries(request);
-                NetworkEvents.getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message});
-                if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) {
-                    NetworkEvents.getLogger().info('Request failed too many times, removing from storage', false, {retryCount, command: request.command, error: error.message});
-                    PersistedRequests.remove(request);
-                }
-            } else if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
-                NetworkEvents.getLogger().info('Persisted request was cancelled. Not retrying.');
-                NetworkEvents.triggerError(request);
-                PersistedRequests.remove(request);
-            } else {
-                NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error while retrying persisted request. Not retrying.`, {
-                    command: request.command,
-                    error: error.message,
-                });
+            const retryCount = PersistedRequests.incrementRetries(request);
+            NetworkEvents.getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message});
+            if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) {
+                NetworkEvents.getLogger().info('Request failed too many times, removing from storage', false, {retryCount, command: request.command, error: error.message});
                 PersistedRequests.remove(request);
             }
         }));

src/libs/Network/index.js

@@ -51,7 +51,7 @@ function retryFailedRequest(queuedRequest, error) {
     }
 
     const retryCount = mainQueueRetryCounter.incrementRetries(queuedRequest);
-    NetworkEvents.getLogger().info('A retryable request failed', false, {
+    NetworkEvents.getLogger().info('[Network] A retryable request failed', false, {
         retryCount,
         command: queuedRequest.command,
         error: error.message,
@@ -62,7 +62,7 @@ function retryFailedRequest(queuedRequest, error) {
         return true;
     }
 
-    NetworkEvents.getLogger().info('Request was retried too many times with no success. No more retries left');
+    NetworkEvents.getLogger().info('[Network] Request was retried too many times with no success. No more retries left');
     return false;
 }
 
@@ -125,32 +125,20 @@ function processNetworkRequestQueue() {
         }
 
         processRequest(queuedRequest)
-            .then(response => NetworkEvents.triggerResponse(queuedRequest, response))
             .catch((error) => {
-                // Cancelled requests are normal and can happen when a user logs out. No extra handling is needed here.
-                if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
-                    NetworkEvents.triggerError(queuedRequest, error);
-                    return;
-                }
-
                 // Because we ran into an error we assume we might be offline and do a "connection" health test
                 NetworkEvents.triggerRecheckNeeded();
+                if (retryFailedRequest(queuedRequest, error)) {
+                    return;
+                }
 
-                // Retry any request that returns a "Failed to fetch" error. Very common if a user is offline or experiencing an unlikely scenario
-                // like incorrect url, bad cors headers returned by the server, DNS lookup failure etc.
-                if (error.message === CONST.ERROR.FAILED_TO_FETCH) {
-                    if (retryFailedRequest(queuedRequest, error)) {
-                        return;
-                    }
-
-                    // We were not able to retry so pass the error to the handler in API.js
-                    NetworkEvents.triggerError(queuedRequest, error);
+                if (queuedRequest.command !== 'Log') {
+                    NetworkEvents.getLogger().hmmm('[Network] Handled error when making request', error);
                 } else {
-                    NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, {
-                        command: queuedRequest.command,
-                        error: error.message,
-                    });
+                    console.debug('[Network] There was an error in the Log API command, unable to log to server!', error);
                 }
+
+                queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE));
             });
     });
 

src/libs/Network/processRequest.js

@@ -1,6 +1,27 @@
+import lodashGet from 'lodash/get';
+import CONST from '../../CONST';
 import HttpUtils from '../HttpUtils';
 import enhanceParameters from './enhanceParameters';
 import * as NetworkEvents from './NetworkEvents';
+import * as PersistedRequests from '../actions/PersistedRequests';
+
+/**
+ * @param {Object} request
+ * @param {Object} parameters
+ */
+function logRequestDetails(request, parameters) {
+    // Don't log about log or else we'd cause an infinite loop
+    if (request.command === 'Log') {
+        return;
+    }
+
+    NetworkEvents.getLogger().info('Making API request', false, {
+        command: request.command,
+        type: request.type,
+        shouldUseSecure: request.shouldUseSecure,
+        rvl: parameters.returnValueList,
+    });
+}
 
 /**
  * @param {Object} request
@@ -11,11 +32,43 @@ import * as NetworkEvents from './NetworkEvents';
  * @returns {Promise}
  */
 export default function processRequest(request) {
+    const persisted = lodashGet(request, 'data.persist', false);
     const finalParameters = enhanceParameters(request.command, request.data);
 
     // When the request goes past a certain amount of time we trigger a re-check of the connection
     const cancelRequestTimeoutTimer = NetworkEvents.startRecheckTimeoutTimer();
-    NetworkEvents.triggerRequestMade(request, finalParameters);
+    logRequestDetails(request, finalParameters);
     return HttpUtils.xhr(request.command, finalParameters, request.type, request.shouldUseSecure)
+        .then((response) => {
+            if (persisted) {
+                PersistedRequests.remove(request);
+            }
+            NetworkEvents.triggerResponse(request, response);
+            return response;
+        })
+        .catch((error) => {
+            if (error.message === CONST.ERROR.FAILED_TO_FETCH) {
+                // Throw when we get a "Failed to fetch" error so we can retry. Very common if a user is offline or experiencing an unlikely scenario like
+                // incorrect url, bad cors headers returned by the server, DNS lookup failure etc.
+                throw error;
+            }
+
+            // Cancelled requests are normal and can happen when a user logs out. No extra handling is needed here besides
+            // remove the request from the PersistedRequests if the request exists.
+            if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
+                NetworkEvents.getLogger().info('[Network] Request canceled', false, request);
+            } else {
+                // If we get any error that is not "Failed to fetch" create GitHub issue so we can handle it. These requests will not be retried.
+                NetworkEvents.getLogger().alert(`${CONST.ERROR.ENSURE_BUGBOT} unknown error caught while processing request`, {
+                    command: request.command,
+                    error: error.message,
+                });
+            }
+
+            // If we did not throw and we have a persisted request that was cancelled or for an unknown error remove it so it is not retried
+            if (persisted) {
+                PersistedRequests.remove(request);
+            }
+        })
         .finally(() => cancelRequestTimeoutTimer());
 }

tests/unit/MigrationTest.js

@@ -63,7 +63,7 @@ describe('MoveToIndexedDb', () => {
 
     it('Should not clear non Onyx keys from localStorage', () => {
         // Given some Onyx and non-Onyx data exists in localStorage
-        localStorage.setItem(ONYXKEYS.SESSION, JSON.stringify({authToken: 'mock-token', loading: false}));
+        localStorage.setItem(ONYXKEYS.SESSION, JSON.stringify({authToken: 'mock-token'}));
         localStorage.setItem('non-onyx-item', 'MOCK');
 
         // When the migration runs