fix browser errors about async responses

fixes #303

The issue was that the deferred listener
abstraction would `return true` (telling the
browser to expect an async response) for every
message, even if the messages were not from the
library. In `wrapStore()`, those messages would be
ignored causing the browser to log an error:

    Error: A listener indicated an asynchronous response by returning true, but the message channel closed before a response was received

This is mostly harmless, but it's better to avoid
the error. In this commit,
`createDeferredListener()` now takes a `filter`
function that determines if the message will be
handled so that messages from outside the library
can be ignored.

This requires a breaking change: moving the
`channelName` argument to `createWrapStore()`.
This is required since the filter function needs
to know which channel to expect before
`wrapStore()` is called.

index.d.ts

@@ -92,7 +92,6 @@ export class Store<S = any, A extends redux.Action = redux.AnyAction> {
 type WrapStore<S, A extends redux.Action = redux.AnyAction> = (
   store: redux.Store<S, A>,
   configuration?: {
-    channelName?: string;
     dispatchResponder?(
       dispatchResult: any,
       send: (response: any) => void
@@ -106,7 +105,9 @@ type WrapStore<S, A extends redux.Action = redux.AnyAction> = (
 export function createWrapStore<
   S,
   A extends redux.Action = redux.AnyAction
->(): WrapStore<S, A>;
+>(configuration?: {
+    channelName?: string;
+}): WrapStore<S, A>;
 
 export function alias(aliases: {
   [key: string]: (action: any) => any;

src/listener.js

@@ -1,8 +1,33 @@
-export const createDeferredListener = () => {
+/**
+ * Returns a listener that can be passed as a callback to browser API . The listener will queue
+ * events until setListener is called.
+ *
+ * @param {Function} filter - A function that filters messages to be handled by
+ * the listener. This is important to avoid telling the browser to expect an
+ * async response when the message is not intended for this listener.
+ *
+ * @example
+ * const filter = (message, sender, sendResponse) => {
+ *   return message.type === "my_type"
+ * }
+ *
+ * const { listener, setListener } = createDeferredListener(filter);
+ * chrome.runtime.onMessage.addListener(listener);
+ *
+ * // Later, set the listener to handle messages
+ * setListener((message, sender, sendResponse) => {
+ *  console.log(message);
+ * });
+ */
+export const createDeferredListener = (filter) => {
   let resolve = () => {};
   const fnPromise = new Promise((resolve_) => (resolve = resolve_));
 
   const listener = (message, sender, sendResponse) => {
+    if (!filter(message, sender, sendResponse)) {
+      return;
+    }
+
     fnPromise.then((fn) => {
       fn(message, sender, sendResponse);
     });

src/wrap-store/wrapStore.js

@@ -45,7 +45,6 @@ const defaultOpts = {
  * @typedef {function} WrapStore
  * @param {Object} store A Redux store
  * @param {Object} options
- * @param {string} options.channelName The name of the channel for this store.
  * @param {function} options.dispatchResponder A function that takes the result
  * of a store dispatch and optionally implements custom logic for responding to
  * the original dispatch message.
@@ -61,33 +60,37 @@ const defaultOpts = {
  * Wraps a Redux store so that proxy stores can connect to it. This function
  * must be called synchronously when the extension loads to avoid dropping
  * messages that woke the service worker.
+ * @param {Object} options
+ * @param {string} options.channelName The name of the channel for this store.
  * @return {WrapStore} The wrapStore function that accepts a Redux store and
  * options. See {@link WrapStore}.
  */
-export default () => {
+export default ({ channelName = defaultOpts.channelName } = defaultOpts) => {
   const browserAPI = getBrowserAPI();
 
+  const filterStateMessages = (message) =>
+    message.type === FETCH_STATE_TYPE && message.channelName === channelName;
+
+  const filterActionMessages = (message) =>
+    message.type === DISPATCH_TYPE && message.channelName === channelName;
+
   // Setup message listeners synchronously to avoid dropping messages if the
   // extension is woken by a message.
-  const stateProviderListener = createDeferredListener();
-  const actionListener = createDeferredListener();
+  const stateProviderListener = createDeferredListener(filterStateMessages);
+  const actionListener = createDeferredListener(filterActionMessages);
 
   browserAPI.runtime.onMessage.addListener(stateProviderListener.listener);
   browserAPI.runtime.onMessage.addListener(actionListener.listener);
 
   return (
     store,
     {
-      channelName = defaultOpts.channelName,
       dispatchResponder = defaultOpts.dispatchResponder,
       serializer = defaultOpts.serializer,
       deserializer = defaultOpts.deserializer,
       diffStrategy = defaultOpts.diffStrategy,
     } = defaultOpts
   ) => {
-    if (!channelName) {
-      throw new Error("channelName is required in options");
-    }
     if (typeof serializer !== "function") {
       throw new Error("serializer must be a function");
     }
@@ -104,26 +107,21 @@ export default () => {
      * Respond to dispatches from UI components
      */
     const dispatchResponse = (request, sender, sendResponse) => {
-      if (
-        request.type === DISPATCH_TYPE &&
-        request.channelName === channelName
-      ) {
-        const action = Object.assign({}, request.payload, {
-          _sender: sender,
-        });
-
-        let dispatchResult = null;
+      //  Only called with messages that pass the filterActionMessages filter.
+      const action = Object.assign({}, request.payload, {
+        _sender: sender,
+      });
 
-        try {
-          dispatchResult = store.dispatch(action);
-        } catch (e) {
-          dispatchResult = Promise.reject(e.message);
-          console.error(e);
-        }
+      let dispatchResult = null;
 
-        dispatchResponder(dispatchResult, sendResponse);
-        return true;
+      try {
+        dispatchResult = store.dispatch(action);
+      } catch (e) {
+        dispatchResult = Promise.reject(e.message);
+        console.error(e);
       }
+
+      dispatchResponder(dispatchResult, sendResponse);
     };
 
     /**
@@ -173,33 +171,27 @@ export default () => {
       channelName, // Notifying what store is broadcasting the state changes
     });
 
-    const withPayloadDeserializer = withDeserializer(deserializer);
-    const shouldDeserialize = (request) =>
-      request.type === DISPATCH_TYPE && request.channelName === channelName;
-
     /**
      * State provider for content-script initialization
      */
     stateProviderListener.setListener((request, sender, sendResponse) => {
+      // This listener is only called with messages that pass filterStateMessages
       const state = store.getState();
 
-      if (
-        request.type === FETCH_STATE_TYPE &&
-        request.channelName === channelName
-      ) {
-        sendResponse({
-          type: FETCH_STATE_TYPE,
-          payload: state,
-        });
-      }
+      sendResponse({
+        type: FETCH_STATE_TYPE,
+        payload: state,
+      });
     });
 
     /**
      * Setup action handler
      */
+    const withPayloadDeserializer = withDeserializer(deserializer);
+
     withPayloadDeserializer(actionListener.setListener)(
       dispatchResponse,
-      shouldDeserialize
+      filterActionMessages
     );
   };
 };

test/listener.test.js

@@ -1,9 +1,14 @@
 import sinon from "sinon";
 import { createDeferredListener } from "../src/listener";
+import should from "should";
+
+const filterAny = () => {
+  return true;
+};
 
 describe("createDeferredListener", () => {
   it("queues calls to the listener", async () => {
-    const { setListener, listener } = createDeferredListener();
+    const { setListener, listener } = createDeferredListener(filterAny);
     const spy = sinon.spy();
 
     // Trigger a couple of events
@@ -17,6 +22,7 @@ describe("createDeferredListener", () => {
     listener("message3", "sender3", "sendResponse3");
     listener("message4", "sender4", "sendResponse4");
 
+    // Wait for promise queue to clear
     await Promise.resolve();
 
     spy.callCount.should.equal(4);
@@ -25,4 +31,28 @@ describe("createDeferredListener", () => {
     spy.getCall(2).args.should.eql(["message3", "sender3", "sendResponse3"]);
     spy.getCall(3).args.should.eql(["message4", "sender4", "sendResponse4"]);
   });
+
+  it("ignores messages that don't pass the filter", async () => {
+    const filter = (message) => {
+      return message === "message";
+    };
+
+    const { setListener, listener } = createDeferredListener(filter);
+    const spy = sinon.spy();
+
+    const result1 = listener("message", "sender", "sendResponse");
+    const result2 = listener("message2", "sender2", "sendResponse2");
+
+    result1.should.eql(true);
+    console.log(result2);
+    should(result2).eql(undefined);
+
+    setListener(spy);
+
+    // Wait for promise queue to clear
+    await Promise.resolve();
+
+    spy.callCount.should.equal(1);
+    spy.getCall(0).args.should.eql(["message", "sender", "sendResponse"]);
+  });
 });

test/wrapStore.test.js

@@ -88,9 +88,9 @@ describe('wrapStore', function () {
     });
 
     it('should dispatch actions received on onMessage to store', async function () {
-      const wrapStore = createWrapStore();
+      const wrapStore = createWrapStore({ channelName });
 
-      wrapStore(store, { channelName });
+      wrapStore(store);
       listeners.onMessage.forEach(l => l(message, sender, callback));
 
       await Promise.resolve();
@@ -106,9 +106,9 @@ describe('wrapStore', function () {
     });
 
     it('should not dispatch actions received on onMessage for other ports', function () {
-      const wrapStore = createWrapStore();
+      const wrapStore = createWrapStore({ channelName });
 
-      wrapStore(store, { channelName });
+      wrapStore(store);
       message.channelName = channelName + '2';
       listeners.onMessage.forEach(l => l(message, sender, callback));
 
@@ -117,9 +117,9 @@ describe('wrapStore', function () {
 
     it('should deserialize incoming messages correctly', async function () {
       const deserializer = sinon.spy(JSON.parse);
-      const wrapStore = createWrapStore();
+      const wrapStore = createWrapStore({ channelName });
 
-      wrapStore(store, { channelName, deserializer });
+      wrapStore(store, { deserializer });
       message.payload = JSON.stringify(payload);
       listeners.onMessage.forEach(l => l(message, sender, callback));
 
@@ -137,9 +137,9 @@ describe('wrapStore', function () {
 
     it('should not deserialize incoming messages for other ports', function () {
       const deserializer = sinon.spy(JSON.parse);
-      const wrapStore = createWrapStore();
+      const wrapStore = createWrapStore({ channelName });
 
-      wrapStore(store, { channelName, deserializer });
+      wrapStore(store, { deserializer });
       message.channelName = channelName + '2';
       message.payload = JSON.stringify(payload);
       listeners.onMessage.forEach(l => l(message, sender, callback));
@@ -172,9 +172,9 @@ describe('wrapStore', function () {
       .onThirdCall().returns(secondState);
 
     const serializer = (payload) => JSON.stringify(payload);
-    const wrapStore = createWrapStore();
+    const wrapStore = createWrapStore({ channelName });
 
-    wrapStore(store, { channelName, serializer });
+    wrapStore(store, { serializer });
 
     // Simulate a state update by calling subscribers
     subscribers.forEach(subscriber => subscriber());
@@ -223,9 +223,9 @@ describe('wrapStore', function () {
       type: 'FAKE_DIFF',
       oldObj, newObj
     }]);
-    const wrapStore = createWrapStore();
+    const wrapStore = createWrapStore({ channelName });
 
-    wrapStore(store, { channelName, diffStrategy });
+    wrapStore(store, { diffStrategy });
 
     // Simulate a state update by calling subscribers
     subscribers.forEach(subscriber => subscriber());
@@ -259,25 +259,25 @@ describe('wrapStore', function () {
 
     it('should throw an error if serializer is not a function', function () {
       should.throws(() => {
-        const wrapStore = createWrapStore();
+        const wrapStore = createWrapStore({ channelName });
 
-        wrapStore(store, { channelName, serializer: "abc" });
+        wrapStore(store, { serializer: "abc" });
       }, Error);
     });
 
     it('should throw an error if deserializer is not a function', function () {
       should.throws(() => {
-        const wrapStore = createWrapStore();
+        const wrapStore = createWrapStore({ channelName });
 
-        wrapStore(store, { channelName, deserializer: "abc" });
+        wrapStore(store, { deserializer: "abc" });
       }, Error);
     });
 
     it('should throw an error if diffStrategy is not a function', function () {
       should.throws(() => {
-        const wrapStore = createWrapStore();
+        const wrapStore = createWrapStore({ channelName });
 
-        wrapStore(store, { channelName, diffStrategy: "abc" });
+        wrapStore(store, { diffStrategy: "abc" });
       }, Error);
     });
   });
@@ -314,9 +314,9 @@ describe('wrapStore', function () {
           }
         }
       };
-      const wrapStore = createWrapStore();
+      const wrapStore = createWrapStore({ channelName });
 
-      wrapStore(store, { channelName });
+      wrapStore(store);
 
       tabResponders.length.should.equal(5);
     },