Bug 1579795 - Part 1: Improve error handling when fetching the manifest r=jdescottes,fluent-reviewers,flod

Differential Revision: https://phabricator.services.mozilla.com/D49594

--HG--
extra : moz-landing-system : lando

devtools/client/application/src/actions/manifest.js

@@ -4,7 +4,9 @@
 
 "use strict";
 
-const { services } = require("../modules/services");
+const { l10n } = require("../modules/l10n");
+
+const { services, ManifestDevToolsError } = require("../modules/services");
 const {
   FETCH_MANIFEST_FAILURE,
   FETCH_MANIFEST_START,
@@ -15,11 +17,19 @@ const {
 function fetchManifest() {
   return async (dispatch, getState) => {
     dispatch({ type: FETCH_MANIFEST_START });
-    const { manifest, errorMessage } = await services.fetchManifest();
-
-    if (!errorMessage) {
+    try {
+      const manifest = await services.fetchManifest();
       dispatch({ type: FETCH_MANIFEST_SUCCESS, manifest });
-    } else {
+    } catch (error) {
+      let errorMessage = error.message;
+
+      // since Firefox DevTools errors may not make sense for the user, swap
+      // their message for a generic one.
+      if (error instanceof ManifestDevToolsError) {
+        console.error(error);
+        errorMessage = l10n.getString("manifest-loaded-devtools-error");
+      }
+
       dispatch({ type: FETCH_MANIFEST_FAILURE, error: errorMessage });
     }
   };

devtools/client/application/src/modules/services.js

@@ -4,6 +4,14 @@
 
 "use strict";
 
+class ManifestDevToolsError extends Error {
+  constructor(...params) {
+    super(...params);
+
+    this.name = "ManifestDevToolsError";
+  }
+}
+
 class Services {
   init(toolbox) {
     this._toolbox = toolbox;
@@ -15,12 +23,25 @@ class Services {
   }
 
   async fetchManifest() {
-    this._assertInit();
+    let response;
 
-    const manifestFront = await this._toolbox.target.getFront("manifest");
-    const response = await manifestFront.fetchCanonicalManifest();
+    try {
+      this._assertInit();
+      const manifestFront = await this._toolbox.target.getFront("manifest");
+      response = await manifestFront.fetchCanonicalManifest();
+    } catch (error) {
+      throw new ManifestDevToolsError(
+        error.message,
+        error.fileName,
+        error.lineNumber
+      );
+    }
 
-    return response;
+    if (response.errorMessage) {
+      throw new Error(response.errorMessage);
+    }
+
+    return response.manifest;
   }
 
   _assertInit() {
@@ -30,5 +51,8 @@ class Services {
   }
 }
 
-// exports a singleton, which will be used across all application panel modules
-exports.services = new Services();
+module.exports = {
+  ManifestDevToolsError,
+  // exports a singleton, which will be used across all application panel modules
+  services: new Services(),
+};

devtools/client/application/test/components/manifest/components_application_panel-ManifestLoader.test.js

@@ -18,6 +18,7 @@ const {
 
 // Import app modules
 const {
+  ManifestDevToolsError,
   services,
 } = require("devtools/client/application/src/modules/services");
 
@@ -52,7 +53,7 @@ describe("ManifestLoader", () => {
   it("loads a manifest when mounted and triggers actions when loading is OK", async () => {
     const fetchManifestSpy = jest
       .spyOn(services, "fetchManifest")
-      .mockResolvedValue({ manifest: MANIFEST_NO_ISSUES, errorMessage: "" });
+      .mockResolvedValue(MANIFEST_NO_ISSUES);
 
     const store = buildStore({});
 
@@ -70,7 +71,7 @@ describe("ManifestLoader", () => {
   it("loads a manifest when mounted and triggers actions when loading fails", async () => {
     const fetchManifestSpy = jest
       .spyOn(services, "fetchManifest")
-      .mockResolvedValue({ manifest: null, errorMessage: "lorem ipsum" });
+      .mockRejectedValue(new Error("lorem ipsum"));
 
     const store = buildStore({});
 
@@ -85,6 +86,30 @@ describe("ManifestLoader", () => {
     fetchManifestSpy.mockRestore();
   });
 
+  it("loads a manifest when mounted and triggers actions when loading fails due to devtools", async () => {
+    const error = new ManifestDevToolsError(":(");
+    const fetchManifestSpy = jest
+      .spyOn(services, "fetchManifest")
+      .mockRejectedValue(error);
+    const consoleErrorSpy = jest
+      .spyOn(console, "error")
+      .mockImplementation(() => {});
+
+    const store = buildStore({});
+
+    shallow(ManifestLoader({ store })).dive();
+    await flushPromises();
+
+    expect(store.getActions()).toEqual([
+      { type: FETCH_MANIFEST_START },
+      { type: FETCH_MANIFEST_FAILURE, error: "manifest-loaded-devtools-error" },
+    ]);
+    expect(consoleErrorSpy).toHaveBeenCalledWith(error);
+
+    fetchManifestSpy.mockRestore();
+    consoleErrorSpy.mockRestore();
+  });
+
   it("renders a message when it is loading", async () => {
     const store = buildStore({ isLoading: true });
     const wrapper = shallow(ManifestLoader({ store })).dive();

devtools/client/locales/en-US/application.ftl

@@ -105,9 +105,14 @@ manifest-loading = Loading manifest…
 # Text displayed when the manifest has been successfully loaded
 manifest-loaded-ok = Manifest loaded.
 
-# Text displayed when there has been an error while trying to load the manifest
+# Text displayed as a caption when there has been an error while trying to
+# load the manifest
 manifest-loaded-error = There was an error while loading the manifest:
 
+# Text displayed as an error when there has been a Firefox DevTools error while
+# trying to load the manifest
+manifest-loaded-devtools-error = Firefox DevTools error
+
 # Text displayed when the page has no manifest available
 manifest-non-existing = No manifest found to inspect.