tshaddix / webext-redux

A set of utilities for building Redux applications in Web Extensions.
MIT License
1.22k stars 180 forks source link

Missing option for state changing affect to only current open tab #200

Open lamvananh opened 5 years ago

lamvananh commented 5 years ago

Currently, when we change a state, it affect to all chrome tabs are opening. I have been researched but found no way to make it affect only to current open tab.

tshaddix commented 5 years ago

Hi @lamvananh! This can be accomplished through how you architect your app rather than it being an added option/complexity to this library. The way I do this (I have this need quite a bit in my apps) is by scoping parts of the state tree to specific tab id keys. For example, let's say I want to show a loading state but only on specific tabs. Instead of structuring my state like this:

{
  isLoading: boolean;
}

I would structure it like this:

{
  isLoadingbyTabId: {
    [tabId: number]: boolean;
  }
}

Then, in my content script or popup script I'd get the current tab id (you can use a package like this to get tabId in a content script or popup and then just filter the state down to my tab id in my UI components. This way, you're still honoring Redux architecture and "one state" for your apps rather than getting into the complexity of hiding state updates and keeping separate states per tab.

kaglowka commented 5 years ago

Hi @lamvananh , @tshaddix I have used an almost identical solution to yours @tshaddix with some additional assumptions. I guess you might have needed them too and just left them out in your answer. I'm writing them just for the record and to make sure that's more or less what you meant. In the other half of this post I'm suggesting to extend this solution.

Additional assumptions to the solution above

popup

background script

Possible problems and improvements

In my first contact with this library (which is really a must while working on a more complex extension!) there was one major piece of the puzzle that was still missing. I believe it is also related to https://github.com/tshaddix/webext-redux/issues/163 (without full understanding of this issue, so I might be wrong).

One major problem I see with the solution you're mentioning @tshaddix (and which I have adopted, too) is performance. Since every state change is propagated to all tabs, you need to use actions very economically; they are very costly for the browser, and there is even a critical frequency you may not exceed (which is not very abstract -- I tested it and in my rather decent developer environment it was ~4 actions per second with some 30 tabs open).

If you use your redux tab state to only keep the most important part of the actual tab state (putting the rest in the local state of React components), I suppose this won't be a problem. However, if your use case favours slightly richer Redux state that reflects per-tab UI state, you might come up against some limitations.

I think it is rather hard to predict when this critical point will be reached. When the problem occurs, there is little that can be done about it. When it turns out in practice, that your application design requires more actions than anticipated, you might need major amendments to your application design / architecture.

Setting aside the actual critical point, I believe it is also fairly possible that such per-tab-state architecture will cause performance problems for the users. It might stretch the boundaries/recommendations set by chrome extension policy. I am bringing this up, since I believe it is just possible to be on the safe side and use per-tab actions freely without practical performance limits.

possible solution

assumptions:

solution

interface State {
    global: GlobalState,
    tabs: {
        [tabId: string]: TabState;
    }
}

By design store.getState().tabs[tabId] will only contain data relevant to the current tab.

open questions

Side note

In my current project we have made a tentative attempt to make a simple patch to webext-redux to implement the behaviour I described above, but the current architecture did not allow it, since the data structures are not exported and cannot be overwritten without locally duplicating the wrapStore code. Ideally, wrapStore could be implemented a class-like structure which could be inherited from/partly overwritten and any part of the messaging routine could be overwritten, so amendments I mentioned above could be introduced without a need to extend webext-redux in its core. Of course, this is a huge advantage of the current implementation, that it is very straightforward -- making this routine more explicit might, again, add some complexity, so I'm not seriously suggesting that, given the amount of work it would cost -- just sharing the thought with you and wondering what are your thoughts on that @tshaddix :)

srvance commented 5 years ago

I've addressed a separation between content and popup redux data needs by double-wrapping my background store with different portNames and putting a serializer on the content wrapper that filters out the unnecessary reducers.

I also considered per-proxy preferences and decided it was more complexity than I was prepared to take on immediately, but that would be really helpful and allow the proxy side to declare exactly what it needs. My serializer solution forces the background wrapping to know what the particular proxy needs.

kaglowka commented 5 years ago

Hi @srvance , I didn't think of using wrapStore twice to split the communication into content script / popup parts. At first sight it seems to eliminate the asymmetry between popup and content script Proxy Stores, so the popup handling doesn't need to be a built-in feature of wrapStore.

I temporarily used custom patchStrategy on the Proxy Store side, with deepDiff on the background script side to decide if the diff needs to be applied for the given tab. This, however, does not limit the number of messages (dispatched to all browser tabs anyway), which, I believe, is a problem of its own.

@srvance, does your serializer solution limit the number of messages sent? (I did read the source some times ago, but I don't remember if that was even possible)

srvance commented 5 years ago

It didn’t reduce the number of messages, but it vastly reduced the size of the updates, so it relieved a lot of memory churn. I only needed 3/20 reducers in content. An empty deep diff message is trivial.

I played with handling it in the deserializer, but the memory churn had already happened.