From c756d6270a5b03ba880c3035386b0b5483da3d0a Mon Sep 17 00:00:00 2001 From: Sidney Nemzer Date: Thu, 19 Sep 2024 21:47:35 -0400 Subject: [PATCH 1/4] 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 | 5 +-- src/listener.js | 28 ++++++++++++++- src/wrap-store/wrapStore.js | 70 ++++++++++++++++--------------------- test/listener.test.js | 32 ++++++++++++++++- test/wrapStore.test.js | 40 ++++++++++----------- 5 files changed, 112 insertions(+), 63 deletions(-) diff --git a/index.d.ts b/index.d.ts index d3623e0..1f475d5 100644 --- a/index.d.ts +++ b/index.d.ts @@ -92,7 +92,6 @@ export class Store { type WrapStore = ( store: redux.Store, configuration?: { - channelName?: string; dispatchResponder?( dispatchResult: any, send: (response: any) => void @@ -106,7 +105,9 @@ type WrapStore = ( export function createWrapStore< S, A extends redux.Action = redux.AnyAction ->(): WrapStore; +>(configuration?: { + channelName?: string; +}): WrapStore; export function alias(aliases: { [key: string]: (action: any) => any; diff --git a/src/listener.js b/src/listener.js index c057c9a..ab13c99 100644 --- a/src/listener.js +++ b/src/listener.js @@ -1,8 +1,34 @@ -export const createDeferredListener = () => { +/** + * Returns a function that can be passed as a listener callback to a 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, define the listener to handle messages. Messages received + * // before this point are queued. + * 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); }); diff --git a/src/wrap-store/wrapStore.js b/src/wrap-store/wrapStore.js index ad7f4ac..a0e01db 100644 --- a/src/wrap-store/wrapStore.js +++ b/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,16 +60,24 @@ 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); @@ -78,16 +85,12 @@ export default () => { 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 ); }; }; diff --git a/test/listener.test.js b/test/listener.test.js index 8f596ff..8bbc6ad 100644 --- a/test/listener.test.js +++ b/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"]); + }); }); diff --git a/test/wrapStore.test.js b/test/wrapStore.test.js index 31be69f..58586e3 100644 --- a/test/wrapStore.test.js +++ b/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); }, From d738b3b48d48b396c39224d2d583aa23826e8830 Mon Sep 17 00:00:00 2001 From: Sidney Nemzer Date: Fri, 18 Oct 2024 21:31:54 -0400 Subject: [PATCH 2/4] 4.0.0 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index b94eed2..3df0264 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "webext-redux", - "version": "3.0.0", + "version": "4.0.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "webext-redux", - "version": "3.0.0", + "version": "4.0.0", "license": "MIT", "dependencies": { "lodash.assignin": "^4.2.0", diff --git a/package.json b/package.json index 9e30572..8cf6d77 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "webext-redux", - "version": "3.0.0", + "version": "4.0.0", "description": "A set of utilities for building Redux applications in Web Extensions.", "main": "lib/index.js", "typings": "./index.d.ts", From 37735a3142ad963df3df5a9872d932ef6e1b8b1a Mon Sep 17 00:00:00 2001 From: Sidney Nemzer Date: Mon, 21 Oct 2024 22:50:00 -0400 Subject: [PATCH 3/4] update ci from node 12, 14, 16 to 18, 20, 22 --- .github/workflows/node.js.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/node.js.yml b/.github/workflows/node.js.yml index 00debd7..e6a2f2f 100644 --- a/.github/workflows/node.js.yml +++ b/.github/workflows/node.js.yml @@ -15,7 +15,7 @@ jobs: strategy: matrix: - node-version: [12.x, 14.x, 16.x] + node-version: [18.x, 20.x, 22.x] # See supported Node.js release schedule at https://nodejs.org/en/about/releases/ steps: From 9725d44360665c51c75d12118cfbc9e4d933cdca Mon Sep 17 00:00:00 2001 From: Sidney Nemzer Date: Mon, 21 Oct 2024 23:03:38 -0400 Subject: [PATCH 4/4] update actions to v4 --- .github/workflows/node.js.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/node.js.yml b/.github/workflows/node.js.yml index e6a2f2f..d528352 100644 --- a/.github/workflows/node.js.yml +++ b/.github/workflows/node.js.yml @@ -19,9 +19,9 @@ jobs: # See supported Node.js release schedule at https://nodejs.org/en/about/releases/ steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Use Node.js ${{ matrix.node-version }} - uses: actions/setup-node@v2 + uses: actions/setup-node@v4 with: node-version: ${{ matrix.node-version }} cache: "npm"