Start using webext-messenger (for background functions and messages) (

#1258)
pixiebrix · Sep 12, 2021 · 6815bc0 · 6815bc0
1 parent 8f5dd35
commit 6815bc0
Show file tree

Hide file tree

Showing 26 changed files with 270 additions and 223 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -129,6 +129,7 @@
     "webext-content-scripts": "^0.9.0",
     "webext-detect-page": "^3.0.2",
     "webext-dynamic-content-scripts": "^8.0.0",
+    "webext-messenger": "^0.4.0",
     "webext-patterns": "^1.1.1",
     "webext-polyfill-kinda": "^0.1.0",
     "webextension-polyfill-ts": "^0.26.0"

diff --git a/src/background.ts b/src/background.ts
@@ -25,6 +25,7 @@ import "@/telemetry/rollbar";
 import "webpack-target-webextension/lib/background";
 import "webext-dynamic-content-scripts";
 
+import "./background/messenger/registration";
 import "./development/autoreload";
 import "./background/installer";
 import "./messaging/external";

diff --git a/src/background/contextMenus.ts b/src/background/contextMenus.ts
@@ -16,7 +16,6 @@
  */
 
 import pTimeout from "p-timeout";
-import { liftBackground } from "@/background/protocol";
 import { browser, Menus, Tabs } from "webextension-polyfill-ts";
 import { isBackgroundPage } from "webext-detect-page";
 import { reportError } from "@/telemetry/logging";
@@ -40,12 +39,12 @@ const MENU_PREFIX = "pixiebrix-";
 const CONTEXT_SCRIPT_INSTALL_MS = 1000;
 const CONTEXT_MENU_INSTALL_MS = 1000;
 
-interface SelectionMenuOptions {
+export type SelectionMenuOptions = {
   extensionId: UUID;
   title: string;
   contexts: Menus.ContextType[];
   documentUrlPatterns: string[];
-}
+};
 
 function makeMenuId(extensionId: UUID): string {
   return `${MENU_PREFIX}${extensionId}`;
@@ -117,7 +116,11 @@ function menuListener(info: Menus.OnClickData, tab: Tabs.Tab) {
  * Uninstall contextMenu for `extensionId`. Returns true if the contextMenu was removed, or false if the contextMenu was
  * not found.
  */
-export async function uninstall(extensionId: UUID): Promise<boolean> {
+export async function uninstallContextMenu({
+  extensionId,
+}: {
+  extensionId: UUID;
+}): Promise<boolean> {
   try {
     const menuItemId = extensionMenuItems.get(extensionId);
 
@@ -141,101 +144,90 @@ export async function uninstall(extensionId: UUID): Promise<boolean> {
   }
 }
 
-/**
- * Uninstall context menu and return whether or not the context menu was uninstalled.
- */
-export const uninstallContextMenu = liftBackground(
-  "UNINSTALL_CONTEXT_MENU",
-  async ({ extensionId }: { extensionId: UUID }) => uninstall(extensionId)
-);
-
-export const ensureContextMenu = liftBackground(
-  "ENSURE_CONTEXT_MENU",
-  async ({
-    extensionId,
-    contexts,
-    title,
-    documentUrlPatterns,
-  }: SelectionMenuOptions) => {
-    if (!extensionId) {
-      throw new Error("extensionId is required");
-    }
+export async function ensureContextMenu({
+  extensionId,
+  contexts,
+  title,
+  documentUrlPatterns,
+}: SelectionMenuOptions) {
+  if (!extensionId) {
+    throw new Error("extensionId is required");
+  }
 
-    // Handle the thundering herd of re-registrations when a contentScript.reactivate is broadcast
-    if (pendingRegistration.has(extensionId)) {
-      console.debug("contextMenu registration pending for %s", extensionId);
+  // Handle the thundering herd of re-registrations when a contentScript.reactivate is broadcast
+  if (pendingRegistration.has(extensionId)) {
+    console.debug("contextMenu registration pending for %s", extensionId);
 
-      // Is it OK to return immediately? Or do we need to track the common promise that all callers should see?
-      return;
-    }
+    // Is it OK to return immediately? Or do we need to track the common promise that all callers should see?
+    return;
+  }
 
-    pendingRegistration.add(extensionId);
+  pendingRegistration.add(extensionId);
 
-    const updateProperties: Menus.UpdateUpdatePropertiesType = {
-      type: "normal",
-      title,
-      contexts,
-      documentUrlPatterns,
-    };
+  const updateProperties: Menus.UpdateUpdatePropertiesType = {
+    type: "normal",
+    title,
+    contexts,
+    documentUrlPatterns,
+  };
 
-    const expectedMenuId = makeMenuId(extensionId);
+  const expectedMenuId = makeMenuId(extensionId);
 
-    try {
-      let menuId = extensionMenuItems.get(extensionId);
+  try {
+    let menuId = extensionMenuItems.get(extensionId);
 
-      if (menuId) {
-        try {
-          await browser.contextMenus.update(menuId, updateProperties);
-          return;
-        } catch (error: unknown) {
-          console.debug("Cannot update context menu", { error });
-        }
-      } else {
-        // Just to be safe if our `extensionMenuItems` bookkeeping is off, remove any stale menu item
-        await browser.contextMenus.remove(expectedMenuId).catch(noop);
+    if (menuId) {
+      try {
+        await browser.contextMenus.update(menuId, updateProperties);
+        return;
+      } catch (error: unknown) {
+        console.debug("Cannot update context menu", { error });
       }
+    } else {
+      // Just to be safe if our `extensionMenuItems` bookkeeping is off, remove any stale menu item
+      await browser.contextMenus.remove(expectedMenuId).catch(noop);
+    }
 
-      // The update failed, or this is a new context menu
-      extensionMenuItems.delete(extensionId);
-
-      // The types of browser.contextMenus.create are wacky. I verified on Chrome that the method does take a callback
-      // even when using the browser polyfill
-      let createdMenuId: string | number;
-      menuId = await new Promise((resolve, reject) => {
-        // `browser.contextMenus.create` returns immediately with the assigned menu id
-        createdMenuId = browser.contextMenus.create(
-          {
-            ...updateProperties,
-            id: makeMenuId(extensionId),
-          },
-          () => {
-            if (browser.runtime.lastError) {
-              reject(new Error(browser.runtime.lastError.message));
-            }
-
-            resolve(createdMenuId);
+    // The update failed, or this is a new context menu
+    extensionMenuItems.delete(extensionId);
+
+    // The types of browser.contextMenus.create are wacky. I verified on Chrome that the method does take a callback
+    // even when using the browser polyfill
+    let createdMenuId: string | number;
+    menuId = await new Promise((resolve, reject) => {
+      // `browser.contextMenus.create` returns immediately with the assigned menu id
+      createdMenuId = browser.contextMenus.create(
+        {
+          ...updateProperties,
+          id: makeMenuId(extensionId),
+        },
+        () => {
+          if (browser.runtime.lastError) {
+            reject(new Error(browser.runtime.lastError.message));
           }
-        );
-      });
 
-      extensionMenuItems.set(extensionId, menuId);
-    } catch (error: unknown) {
-      if (
-        getErrorMessage(error).includes("Cannot create item with duplicate id")
-      ) {
-        // Likely caused by a concurrent update. In practice, our `pendingRegistration` set and `extensionMenuItems`
-        // should prevent this from happening
-        console.debug("Error registering context menu item", { error });
-        return;
-      }
+          resolve(createdMenuId);
+        }
+      );
+    });
 
-      console.error("Error registering context menu item", { error });
-      throw error;
-    } finally {
-      pendingRegistration.delete(extensionId);
+    extensionMenuItems.set(extensionId, menuId);
+  } catch (error: unknown) {
+    if (
+      getErrorMessage(error).includes("Cannot create item with duplicate id")
+    ) {
+      // Likely caused by a concurrent update. In practice, our `pendingRegistration` set and `extensionMenuItems`
+      // should prevent this from happening
+      console.debug("Error registering context menu item", { error });
+      return;
     }
+
+    console.error("Error registering context menu item", { error });
+    throw error;
+  } finally {
+    pendingRegistration.delete(extensionId);
   }
-);
+}
 
 if (isBackgroundPage()) {
   browser.contextMenus.onClicked.addListener(menuListener);

diff --git a/src/background/deployment.ts b/src/background/deployment.ts
@@ -29,8 +29,10 @@ import { refreshRegistries } from "@/hooks/useRefresh";
 import { liftBackground } from "@/background/protocol";
 import * as contentScript from "@/contentScript/lifecycle";
 import { selectExtensions } from "@/options/selectors";
-import { uninstallContextMenu } from "@/background/contextMenus";
-import { containsPermissions } from "@/utils/permissions";
+import {
+  uninstallContextMenu,
+  containsPermissions,
+} from "@/background/messenger/api";
 import { deploymentPermissions } from "@/permissions";
 import { IExtension } from "@/core";
 import { ExtensionOptionsState } from "@/store/extensions";
@@ -83,9 +85,7 @@ function installDeployment(
         extensionId: extension.id,
       };
 
-      void uninstallContextMenu(identifier).catch((error) => {
-        reportError(error);
-      });
+      void uninstallContextMenu(identifier).catch(reportError);
 
       returnState = reducer(returnState, actions.removeExtension(identifier));
     }

diff --git a/src/background/devtools/protocol.ts b/src/background/devtools/protocol.ts
@@ -31,7 +31,6 @@ import {
 } from "@/background/devtools/internal";
 import { ensureContentScript } from "@/background/util";
 import { isEmpty } from "lodash";
-import * as contextMenuProtocol from "@/background/contextMenus";
 import { Target } from "@/background/devtools/contract";
 import { DynamicDefinition } from "@/nativeEditor/dynamic";
 import { RegistryId, UUID } from "@/core";
@@ -206,14 +205,6 @@ export const runReader = liftBackground(
     })
 );
 
-export const uninstallContextMenu = liftBackground(
-  "UNINSTALL_CONTEXT_MENU",
-  // False positive - it's the inner method that should be async
-  // eslint-disable-next-line unicorn/consistent-function-scoping
-  () => async ({ extensionId }: { extensionId: UUID }) =>
-    contextMenuProtocol.uninstall(extensionId)
-);
-
 export const uninstallActionPanelPanel = liftBackground(
   "UNINSTALL_ACTION_PANEL_PANEL",
   // False positive - it's the inner method that should be async