zalmoxisus / crossbuilder

Building web, Electron, Cordova and Chrome apps, and cross-browser extensions with React, Redux and Webpack. "Write once, deploy everywhere" concept in practice.
MIT License
484 stars 50 forks source link

Use one store without rehydratations #19

Closed zalmoxisus closed 9 years ago

zalmoxisus commented 9 years ago

As discussed in #18 and #16, in order to increase the performance and to avoid errors, we want to deprecate browser-redux-bg and just use background page's store in other extension pages.

So, there will be no more a call in background action, as all redux actions will be dispatched in background. It's very convenient as you may dispatch an action from everywhere, and even if you close the popup or the injected page, you will get it done.

To implement it, we need to:

Any suggestions are welcome.

rt2zz commented 9 years ago

:+1: While I am biased to the old implementation, moving the store to the background page would be much better conceptually and presumably performance wise as well.

Question: how would you deal with page specific state? Specifically I think this might occur with content scripts where you want independent state per open tab. Perhaps a nested store keyed by tab id, and then include tab id in all relevant actions? e.g. dispatch({type: 'SOME_ACTION', tabId: [tabId], payload: payload})

zalmoxisus commented 9 years ago

That's a good question, and yes, I was thinking about the same solution. Having the tabId in the reducer will make it easier to debug, and it will be in the flux way. Other approaches generates cascading effect, opposite to the flux architecture.

The implementation for content scripts is still an open issue as we cannot get the store variable directly from the background script here (chrome.runtime.getBackgroundPage is not allowed in the injected script). We want to achieve it via messaging, which will be slower (though an open connection should make it faster), and probably not usable if we have states for the UI (it needs some tests). So, the solution would be just not to use states for everything or not to use the global store for this case.

Also, except Chrome, we cannot persist states in the injected script as actually the localStore will be for that site not for our extension. In case of Firefox it is just a temporary issue, but it will remain for Safari. Getting the store via messaging would solve this.

We will still use redux-persist in order to not loose the states when the browser is closed, but don't have to sync states between pages/tabs anymore.

I'm still thinking how to organize this better, and am open to any suggestions.

jhen0409 commented 9 years ago

:+1: It's good idea! :)

I think messaging is a good way, but it will not have store instance in UI. Maybe we should to implement store functions wrapper in UI?

zalmoxisus commented 9 years ago

Yes, I think better not to implement my proposal for content scripts. So, in the injected page we will have a different implementation of Redux store (without syncing), where we may store its UI states as well. Sadly, the content script runs in the different process than extension's pages (background, popup, windows, tabs) and we'll not get chrome.runtime.getBackgroundPage from there. Making a workaround with messaging will make it slow for most of cases. We'll just implement an example of the content script to communicate with the background page, not to sync the states directly.

The popup page, extension tabs and windows pages will use the same global store from background.

zalmoxisus commented 9 years ago

As intended, the extension's popup, windows and tabs now refer the store from the background page. So, now we have one store for the whole application, except content scripts.

Each content script has its store, and it can be synchronized with the application (background store) and other content scripts. Unlike rehydratations, we synchronize actions not states. So it may have even different structure of the states.

To implement that I created the following modules:

The store states for the background page are persistent (with redux-persist).

The current implementation breaks firefox support, but hopefully will be implemented soon. It should be tracked in #12. For Safari we already have safari.extension.globalPage.contentWindow.

Any feedback is welcome. I'll leave this issue open till the next release. We need documentation for the above modules and the changes, but it should work as intended.

zalmoxisus commented 9 years ago

Implemented in 0.5.0.