Merge branch '335530-suppress-cancelled-request-errors-in-apollo' into 'master'

Suppress network errors in Apollo during naviation

See merge request gitlab-org/gitlab!72031

app/assets/javascripts/lib/apollo/suppress_network_errors_during_navigation_link.js

+import { Observable } from 'apollo-link';
+import { onError } from 'apollo-link-error';
+import { isNavigatingAway } from '~/lib/utils/is_navigating_away';
+
+/**
+ * Returns an ApolloLink (or null if not enabled) which supresses network
+ * errors when the browser is navigating away.
+ *
+ * @returns {ApolloLink|null}
+ */
+export const getSuppressNetworkErrorsDuringNavigationLink = () => {
+  if (!gon.features?.suppressApolloErrorsDuringNavigation) {
+    return null;
+  }
+
+  return onError(({ networkError }) => {
+    if (networkError && isNavigatingAway()) {
+      // Return an observable that will never notify any subscribers with any
+      // values, errors, or completions. This ensures that requests aborted due
+      // to navigating away do not trigger any failure behaviour.
+      //
+      // See '../utils/suppress_ajax_errors_during_navigation.js' for an axios
+      // interceptor that performs a similar role.
+      return new Observable(() => {});
+    }
+
+    // We aren't suppressing anything here, so simply do nothing.
+    // The onError helper will forward all values/errors/completions from the
+    // underlying request observable to the next link if you return a falsey
+    // value.
+    //
+    // Note that this return statement is technically redundant, but is kept
+    // for explicitness.
+    return undefined;
+  });
+};

app/assets/javascripts/lib/graphql.js

@@ -11,6 +11,7 @@ import csrf from '~/lib/utils/csrf';
 import { objectToQuery, queryToObject } from '~/lib/utils/url_utility';
 import PerformanceBarService from '~/performance_bar/services/performance_bar_service';
 import { getInstrumentationLink } from './apollo/instrumentation_link';
+import { getSuppressNetworkErrorsDuringNavigationLink } from './apollo/suppress_network_errors_during_navigation_link';
 
 export const fetchPolicies = {
   CACHE_FIRST: 'cache-first',
@@ -143,6 +144,7 @@ export default (resolvers = {}, config = {}) => {
     new ActionCableLink(),
     ApolloLink.from(
       [
+        getSuppressNetworkErrorsDuringNavigationLink(),
         getInstrumentationLink(),
         requestCounterLink,
         performanceBarLink,

app/assets/javascripts/lib/utils/axios_utils.js

@@ -2,6 +2,7 @@ import axios from 'axios';
 import { registerCaptchaModalInterceptor } from '~/captcha/captcha_modal_axios_interceptor';
 import setupAxiosStartupCalls from './axios_startup_calls';
 import csrf from './csrf';
+import { isNavigatingAway } from './is_navigating_away';
 import suppressAjaxErrorsDuringNavigation from './suppress_ajax_errors_during_navigation';
 
 axios.defaults.headers.common[csrf.headerKey] = csrf.token;
@@ -30,16 +31,11 @@ axios.interceptors.response.use(
   },
 );
 
-let isUserNavigating = false;
-window.addEventListener('beforeunload', () => {
-  isUserNavigating = true;
-});
-
 // Ignore AJAX errors caused by requests
 // being cancelled due to browser navigation
 axios.interceptors.response.use(
   (response) => response,
-  (err) => suppressAjaxErrorsDuringNavigation(err, isUserNavigating),
+  (err) => suppressAjaxErrorsDuringNavigation(err, isNavigatingAway()),
 );
 
 registerCaptchaModalInterceptor(axios);

app/assets/javascripts/lib/utils/is_navigating_away.js

+let navigating = false;
+
+window.addEventListener('beforeunload', () => {
+  navigating = true;
+});
+
+/**
+ * To only be used for testing purposes. Allows the navigating state to be set
+ * to a given value.
+ *
+ * @param {boolean} value The value to set the navigating flag to.
+ */
+export const setNavigatingForTestsOnly = (value) => {
+  navigating = value;
+};
+
+/**
+ * Returns a boolean indicating whether the browser is in the process of
+ * navigating away from the current page.
+ *
+ * @returns {boolean}
+ */
+export const isNavigatingAway = () => navigating;

config/feature_flags/development/suppress_apollo_errors_during_navigation.yml

+---
+name: suppress_apollo_errors_during_navigation
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72031
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/342745
+milestone: '14.4'
+type: development
+group: group::foundations
+default_enabled: false

lib/gitlab/gon_helper.rb

@@ -55,6 +55,7 @@ module Gitlab
       push_frontend_feature_flag(:security_auto_fix, default_enabled: false)
       push_frontend_feature_flag(:improved_emoji_picker, default_enabled: :yaml)
       push_frontend_feature_flag(:new_header_search, default_enabled: :yaml)
+      push_frontend_feature_flag(:suppress_apollo_errors_during_navigation, current_user, default_enabled: :yaml)
     end
 
     # Exposes the state of a feature flag to the frontend code.

package.json

@@ -99,6 +99,7 @@
     "apollo-client": "^2.6.10",
     "apollo-link": "^1.2.14",
     "apollo-link-batch-http": "^1.2.14",
+    "apollo-link-error": "^1.1.13",
     "apollo-link-http": "^1.5.17",
     "apollo-upload-client": "^13.0.0",
     "autosize": "^4.0.2",

spec/frontend/lib/apollo/suppress_network_errors_during_navigation_link_spec.js

+import { ApolloLink, Observable } from 'apollo-link';
+import waitForPromises from 'helpers/wait_for_promises';
+import { getSuppressNetworkErrorsDuringNavigationLink } from '~/lib/apollo/suppress_network_errors_during_navigation_link';
+import { isNavigatingAway } from '~/lib/utils/is_navigating_away';
+
+jest.mock('~/lib/utils/is_navigating_away');
+
+describe('getSuppressNetworkErrorsDuringNavigationLink', () => {
+  const originalGon = window.gon;
+  let subscription;
+
+  beforeEach(() => {
+    window.gon = originalGon;
+  });
+
+  afterEach(() => {
+    if (subscription) {
+      subscription.unsubscribe();
+    }
+  });
+
+  const makeMockGraphQLErrorLink = () =>
+    new ApolloLink(() =>
+      Observable.of({
+        errors: [
+          {
+            message: 'foo',
+          },
+        ],
+      }),
+    );
+
+  const makeMockNetworkErrorLink = () =>
+    new ApolloLink(
+      () =>
+        new Observable(() => {
+          throw new Error('NetworkError');
+        }),
+    );
+
+  const makeMockSuccessLink = () =>
+    new ApolloLink(() => Observable.of({ data: { foo: { id: 1 } } }));
+
+  const createSubscription = (otherLink, observer) => {
+    const mockOperation = { operationName: 'foo' };
+    const link = getSuppressNetworkErrorsDuringNavigationLink().concat(otherLink);
+    subscription = link.request(mockOperation).subscribe(observer);
+  };
+
+  describe('when disabled', () => {
+    it('returns null', () => {
+      expect(getSuppressNetworkErrorsDuringNavigationLink()).toBe(null);
+    });
+  });
+
+  describe('when enabled', () => {
+    beforeEach(() => {
+      window.gon = { features: { suppressApolloErrorsDuringNavigation: true } };
+    });
+
+    it('returns an ApolloLink', () => {
+      expect(getSuppressNetworkErrorsDuringNavigationLink()).toEqual(expect.any(ApolloLink));
+    });
+
+    describe('suppression case', () => {
+      describe('when navigating away', () => {
+        beforeEach(() => {
+          isNavigatingAway.mockReturnValue(true);
+        });
+
+        describe('given a network error', () => {
+          it('does not forward the error', async () => {
+            const spy = jest.fn();
+
+            createSubscription(makeMockNetworkErrorLink(), {
+              next: spy,
+              error: spy,
+              complete: spy,
+            });
+
+            // It's hard to test for something _not_ happening. The best we can
+            // do is wait a bit to make sure nothing happens.
+            await waitForPromises();
+            expect(spy).not.toHaveBeenCalled();
+          });
+        });
+      });
+    });
+
+    describe('non-suppression cases', () => {
+      describe('when not navigating away', () => {
+        beforeEach(() => {
+          isNavigatingAway.mockReturnValue(false);
+        });
+
+        it('forwards successful requests', (done) => {
+          createSubscription(makeMockSuccessLink(), {
+            next({ data }) {
+              expect(data).toEqual({ foo: { id: 1 } });
+            },
+            error: () => done.fail('Should not happen'),
+            complete: () => done(),
+          });
+        });
+
+        it('forwards GraphQL errors', (done) => {
+          createSubscription(makeMockGraphQLErrorLink(), {
+            next({ errors }) {
+              expect(errors).toEqual([{ message: 'foo' }]);
+            },
+            error: () => done.fail('Should not happen'),
+            complete: () => done(),
+          });
+        });
+
+        it('forwards network errors', (done) => {
+          createSubscription(makeMockNetworkErrorLink(), {
+            next: () => done.fail('Should not happen'),
+            error: (error) => {
+              expect(error.message).toBe('NetworkError');
+              done();
+            },
+            complete: () => done.fail('Should not happen'),
+          });
+        });
+      });
+
+      describe('when navigating away', () => {
+        beforeEach(() => {
+          isNavigatingAway.mockReturnValue(true);
+        });
+
+        it('forwards successful requests', (done) => {
+          createSubscription(makeMockSuccessLink(), {
+            next({ data }) {
+              expect(data).toEqual({ foo: { id: 1 } });
+            },
+            error: () => done.fail('Should not happen'),
+            complete: () => done(),
+          });
+        });
+
+        it('forwards GraphQL errors', (done) => {
+          createSubscription(makeMockGraphQLErrorLink(), {
+            next({ errors }) {
+              expect(errors).toEqual([{ message: 'foo' }]);
+            },
+            error: () => done.fail('Should not happen'),
+            complete: () => done(),
+          });
+        });
+      });
+    });
+  });
+});

spec/frontend/lib/utils/is_navigating_away_spec.js

+import { isNavigatingAway, setNavigatingForTestsOnly } from '~/lib/utils/is_navigating_away';
+
+describe('isNavigatingAway', () => {
+  beforeEach(() => {
+    // Make sure each test starts with the same state
+    setNavigatingForTestsOnly(false);
+  });
+
+  it.each([false, true])('it returns the navigation flag with value %s', (flag) => {
+    setNavigatingForTestsOnly(flag);
+    expect(isNavigatingAway()).toEqual(flag);
+  });
+
+  describe('when the browser starts navigating away', () => {
+    it('returns true', () => {
+      expect(isNavigatingAway()).toEqual(false);
+
+      window.dispatchEvent(new Event('beforeunload'));
+
+      expect(isNavigatingAway()).toEqual(true);
+    });
+  });
+});

yarn.lock

@@ -2279,6 +2279,15 @@ apollo-link-batch@^1.1.15:
     apollo-link "^1.2.14"
     tslib "^1.9.3"
 
+apollo-link-error@^1.1.13:
+  version "1.1.13"
+  resolved "https://registry.yarnpkg.com/apollo-link-error/-/apollo-link-error-1.1.13.tgz#c1a1bb876ffe380802c8df0506a32c33aad284cd"
+  integrity sha512-jAZOOahJU6bwSqb2ZyskEK1XdgUY9nkmeclCrW7Gddh1uasHVqmoYc4CKdb0/H0Y1J9lvaXKle2Wsw/Zx1AyUg==
+  dependencies:
+    apollo-link "^1.2.14"
+    apollo-link-http-common "^0.2.16"
+    tslib "^1.9.3"
+
 apollo-link-http-common@^0.2.14, apollo-link-http-common@^0.2.16:
   version "0.2.16"
   resolved "https://registry.yarnpkg.com/apollo-link-http-common/-/apollo-link-http-common-0.2.16.tgz#756749dafc732792c8ca0923f9a40564b7c59ecc"