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

Thoughts on multi store support #8

Closed
maxwellmstein opened this issue Jul 12, 2017 · 12 comments
Closed

Thoughts on multi store support #8

maxwellmstein opened this issue Jul 12, 2017 · 12 comments

Comments

@maxwellmstein
Copy link

Thought I'd put this out here and see what people thought.

It looks like right now the redux component is relying on the window as the central communication point. In order to support multiple redux-enabled 'applications' we need to find a way to control what is dispatched to who.

My instinct would be to have use some sort of namespace attribute on the component, that connecting components would reference.

<c:reduxStore aura:id="store" ns="todo1" />

then the store cmp could just keep track of windows.stores[ns]. I assume this could get a bit messy with the sub/dis queues as well, but could be handled easily enough.

I'm happy to implement and PR this back if there is interest, just wanted to see if any other approaches warranted discussion.

@madmax983
Copy link
Owner

In terms of technical Salesforce namespaces setup by different managed packages, LockerService should take care of this. Each namespace will get it's own SecureWindow, and thus each instance of redux in that namespace will sandboxed off from each other. Unless my understanding of the LockerService architecture is completely off. :)

In terms of having something like multi-store support within one namespace, I would be open to it. I could see where you might have two very distinct "apps" in LEX, and truly want to keep them separate from each other.

Aside: That was actually one of my motivations for making the library compatible with reselect. You can put the Counter component and Todo Component on the same LEX page, and with reselect, updates on the counter won't cause the todo's filter computed function to fire. Otherwise I could see having a ton of redux-enabled cmp's on the same LEX page running into some eventual perf problems.

@maxwellmstein
Copy link
Author

OK - the other angle comes from the LEX not destroying components on aura:locationChange (for faster back-buttoning) In the past I've worked around it by wiring my components to the locationChange event and manually cmp.destroy() there, but there are random side-effects if you use your component outside the LEX as well. Ends up getting ugly.

In my case, I will have multiple redux apps within the same page for sure, or instances of the same app twice so I'm going to go down route of keeping the stores namespaced on the window. I'll commit to my forked repo after and come of it what may.

@maniax89
Copy link

just chiming in here a year later, have been using this library and have recently run into the situation described in #8 (comment)

In my case, I will have multiple redux apps within the same page for sure, or instances of the same app twice so I'm going to go down route of keeping the stores namespaced on the window.

@maxwellmstein did you ever end up pushing those changes?

@madmax983
Copy link
Owner

@maniax89 Activity! I see the repo getting cloned in the traffic, so it's nice to put a username and face to one of those number. As far as I know, max never made any headway in this regard.

I actually had a POC for this that I was working on, but stopped because I don't think I understand the use-case well enough. My understanding is you generally want to keep everything in one store, and use reducer composition for modularity (This stackoverflow has a great discussion on the topic).

The POC I had basically let you register the name of the store versus just having window.reduxStore that the component is currently utilizing. But I didn't have a use case where this solved anything over reducer composition.

madmax983 pushed a commit that referenced this issue Aug 20, 2018
@maniax89
Copy link

i suppose i could keep track of the globalId https://developer.salesforce.com/docs/atlas.en-us.lightning.meta/lightning/components_ids.htm and pass that down throughout the child component hierarchy in order to solve the duplicate component on the same page problem, but seems like a use-case that could potentially have a cleaner solution :-/

I am totally behind the single store approach aside from the intent to run the same application multiple times on the same page. I will use the global ID namespacing in the reducer names for now

@madmax983
Copy link
Owner

madmax983 commented Aug 20, 2018

Yeah, this isn't totally a new problem for Redux in general: reduxjs/redux#1602

I am totally open to suggestions on how to address it with Lightning-Redux. I also plopped my POC in the multiple-stores branch if you want to take a peek (warning: super untested code). I am also unsure that the approach taken there will really solve the problem, since you would still need to dynamically generate the store name per component, which would probably involve the globalID.

@maniax89
Copy link

I think the multiple stores approach you posted would enable myself and others to provide a unique identifier as the name attribute of the store when instantiated, which should solve the problem that I am seeing with multiple instances of the same application on the page

@maniax89
Copy link

@madmax983 i opened a new PR to build upon the POC you had working #15

@madmax983
Copy link
Owner

@maniax89 Thank you for the PR, and for taking a look at the in progress POC. Your changes make sense, so I will pull those in, but I will want to write up so more robust unit tests before pushing it into master.

@madmax983
Copy link
Owner

Got all of this running up in an org (I miss having CI setup, but they aren't doing personal Dev Hubs, and don't want to hook it into the business).

Looks like the queues are not currently working with this approach. I'll take a look at it tonight and see if I can figure out what is going on.
screen shot 2018-08-27 at 10 15 02 am

madmax983 pushed a commit that referenced this issue Aug 27, 2018
@maniax89
Copy link

@madmax983 apologies for that - to be honest i didn't look at the queues in my PR since I wasn't using them for the implementation.

I totally agree with updating tests first - sorry I didn't do it myself. If you don't get to it by the end of the week I might try and take a stab at it this weekend

madmax983 pushed a commit that referenced this issue Aug 27, 2018
…nt options. Checking in mdapi format metadata.
@madmax983
Copy link
Owner

madmax983 commented Aug 27, 2018

Managed to get this done on lunch, and merged into master. Let me know if there are any issues!

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

No branches or pull requests

3 participants