Whenever the app tabs remove/reload, the old subscribers is not removed from the store

[phucpnt]

Support new function for unsubscribe the old listeners when app tabs is close/reload

Pls let me know Phuc

[zalmoxisus]

That's a good catch and great solution! A good addition would be having listeners only when it is required (so the extension will not do anything when not used) and removing listeners when they are not needed anymore. It can be implemented using chrome.tabs.onRemoved.hasListeners (/onUpdated) and chrome.tabs.onRemoved.removeListener as we do with windows here.

Also it seems there are some ESLint conflicts. I'll refactor this when I get some time. If you implement this, it will be welcome!

Thanks again for the fix!

[phucpnt]

Thanks @zalmoxisus

I has updated the source to fix the eslint issue. Initially, the unsubscribing only happens for the tab which use the store. I don't see where we need to check the hasListeners() from background. Can you show me?

[zalmoxisus]

Hey @phucpnt,

I'm sorry for taking it so long. I didn't mean checking listener for the store (its ok in your implementation), but for chrome.tabs.onRemoved. I will add the fixes in few days, being very busy right now. But you may try to add this if you want.

[phucpnt]

Hello @zalmoxisus Thanks for your clarifications, I has updated the code.

Let me know if that help

[zalmoxisus]

Thanks for the changes, but the idea was not to listen every tab before it is necessary (before makeStore is called). In your case hasListeners will be checked on startup and will be always true.

To be clear I like your idea of makeStore for keeping unsubscribing callers there. The only fact that concerns me is that the background script will always listen for every tab.

I'm sorry for taking your time, but I'd suggest to close this pr and open another one (not to have the previous changes committed) with the following:

• Put tabs listeners in a separate function (so it is easier to remove listener and just not to repeat yourself) and also remove these tab listeners when unsubscribesList becomes empty:

function tabListener(tabId, changeInfo) => {
    if (unsubscribesList[tabId] && (!changeInfo || changeInfo.status === 'loading')) {
       unsubscribesList[tabId]();
       delete unsubscribesList[tabId];
       if (Object.getOwnPropertyNames(unsubscribesList).length === 0) {
         chrome.tabs.onRemoved.removeListener(tabListener);
         chrome.tabs.onUpdated.removeListener(tabListener);
       }
     }
}

• As the result window.makeStore would be:

  window.makeStore = (tabId) => {
    if (!chrome.tabs.onRemoved.hasListeners()) chrome.tabs.onRemoved.addListener(tabListener);
    if (!chrome.tabs.onUpdated.hasListeners()) chrome.tabs.onUpdated.addListener(tabListener);

    const storeSubscribe = store.subscribe;
    return { ...store,
      subscribe(...args) {
        const unsubscribe = storeSubscribe(...args);
        unsubscribesList[tabId] = unsubscribe;
        return unsubscribe;
      }
    };
  };

I didn't check the code, and something could be missed, but hope you get the idea.

[zalmoxisus]

What about the cases when having more than one subscriber to the store in one tab?

Probably, instead of unsubscribesList[tabId] = unsubscribe; we should use

if (unsubscribesList[tabId]) unsubscribesList[tabId].push(unsubscribe);
else unsubscribesList[tabId] = [unsubscribe];

And unsubscribesList[tabId](); would become:

unsubscribesList[tabId].forEach(unsubscribe => { unsubscribe(); });

What do you think about this?

[phucpnt]

Thanks @zalmoxisus

I think your point is correct and insightful for the chrome tab listeners

In case, there is more than one subscriber in one tab, I'm ok with your solution. I think the issue is closed for me, would you or I complete the final code updates?

Let me know

[zalmoxisus]

If you have time to submit a new pull request and close this one (not to have the previous commits), it would be welcome. If not, I will do it in few days. Thanks for the changes and patience!

[phucpnt]

Thanks @zalmoxisus

I will close this pull and start a new request soon, if you didn't update the code by then :)

[zalmoxisus]

Hey @phucpnt,

How do you pass tabId to makeStore from your window's script?

[zalmoxisus]

I don't see how easily to get tab id from the window's page. So, it would be better to get it from the makeStore function like:

window.makeStore = () {
    ...
  chrome.tabs.query({
    active: true,
    windowId: chrome.windows.WINDOW_ID_CURRENT
  }, (tabId) => {
    const storeSubscribe = store.subscribe;
    cb({ ...store,
      subscribe(...args) {
        const unsubscribe = storeSubscribe(...args);
        unsubscribeList[tabId] = unsubscribe;
        return unsubscribe;
      }
    });
  });
};

Also, the suggested solution wouldn't work for browser/pageAction popup as it doesn't trigger chrome.tabs.onRemoved / onUpdated events. So, we would probably introduce a window.unsubscribeFromStore function and call it from window.onbeforeunload from the window's script. This also will remove some boilerplate I've introduced.

[phucpnt]

I prefer background will handle the unsubscribe for the store because it introduce the makeStore. So that the app would use the wrappedStore as the same with originStore. The unsubscribedFromStore is a good point, but if we force the app would do the unsubscribed explicitly, i think we lose the simplicity for the wrappedStore compare to originStore.

Let me know what you think.

BTW, do we have the equivalent mechanism with the pageAction popup as chrome.tabs.onRemoved / onUpdated ?

[zalmoxisus]

BTW, do we have the equivalent mechanism with the pageAction popup as chrome.tabs.onRemoved / onUpdated ?

Unfortunately, there aren't any. We could watch clicking on the pageAction / browserAction button, but the popup can become hidden not only by clicking on the button. So, we need to bind unload event from inside the window.

I've come out with the solution in https://github.com/zalmoxisus/crossbuilder/commit/70a804f167fe4c24da328c54ba5e4d311f902ee8. Please give it a try and let me know whether it works for you.

[phucpnt]

It's great update @zalmoxisus

Only one thing I would add is deleting the unsubscribeList.

window.getStore = () => {
    let unsubscribeList = [];
    return {
      store: {
        ...store,
        subscribe(...args) {
          const unsubscribe = store.subscribe(...args);
          unsubscribeList.push(unsubscribe);
          return unsubscribe;
        }
      },
      unsubscribe: () => {
        unsubscribeList.forEach(unsubscriber => { unsubscriber(); });
        unsubscribeList.length = 0;
      }
    };
  };

[zalmoxisus]

I'm not sure setting the array's length to 0 removes it. Do you have any references about this? Easier: unsubscribeList = undefined or unsubscribeList = [].

Anyway, unsubscribeList is a local variable, and garbage collector should remove it, doesn't it?

[phucpnt]

​As long as there is not reference to unsubscribeList, the gc would remove it if necessary. And it is not related to local variable as I understand. Assign length=0 is just a way to empty array. Please see that http://stackoverflow.com/questions/1232040/how-do-i-empty-an-array-in-javascript

---

Phuc PNT.

On Thu, Mar 10, 2016 at 7:20 PM, Mihail Diordiev notifications@github.com wrote:

I'm not sure setting the array's length to 0 removes it. Do you have any references about this? Easier: unsubscribeList = undefined.

Anyway, unsubscribeList is a local variable, and garbage collector should remove it, doesn't it?

— Reply to this email directly or view it on GitHub https://github.com/zalmoxisus/crossbuilder/pull/34#issuecomment-194817417 .

[zalmoxisus]

Every window will call getStore and will have its local unsubscribeList, so it should be destroyed along with anything else in that window, though I don't see a way to check it.

[phucpnt]

Yes, I agree with you. I think I has over optimize in this case.