Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use one store without rehydratations #19

Closed
3 tasks done
zalmoxisus opened this issue Nov 5, 2015 · 6 comments
Closed
3 tasks done

Use one store without rehydratations #19

zalmoxisus opened this issue Nov 5, 2015 · 6 comments
Milestone

Comments

@zalmoxisus
Copy link
Owner

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:

  • remove rehydratations (change browser-redux-sync).
  • get the store with chrome.runtime.getBackgroundPage for all the pages except the background.
  • make a workaround for the content script via messaging.

Any suggestions are welcome.

@zalmoxisus zalmoxisus added this to the v0.5.0 milestone Nov 5, 2015
@rt2zz
Copy link
Contributor

rt2zz commented Nov 5, 2015

👍 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
Copy link
Owner Author

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
Copy link
Collaborator

jhen0409 commented Nov 6, 2015

👍 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
Copy link
Owner Author

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 added a commit that referenced this issue Nov 8, 2015
zalmoxisus added a commit that referenced this issue Nov 13, 2015
zalmoxisus added a commit that referenced this issue Nov 14, 2015
zalmoxisus added a commit that referenced this issue Nov 14, 2015
zalmoxisus added a commit that referenced this issue Nov 14, 2015
@zalmoxisus
Copy link
Owner Author

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
Copy link
Owner Author

Implemented in 0.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants