madmax983 / lightning-redux

Lightning Components and Redux: A Predictable State Container for Lightning Apps
MIT License
43 stars 8 forks source link

Thoughts on multi store support #8

Closed maxwellmstein closed 6 years ago

maxwellmstein commented 7 years ago

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.

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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

just chiming in here a year later, have been using this library and have recently run into the situation described in https://github.com/madmax983/lightning-redux/issues/8#issuecomment-317085379

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 commented 6 years ago

@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.

maniax89 commented 6 years ago

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 commented 6 years ago

Yeah, this isn't totally a new problem for Redux in general: https://github.com/reduxjs/redux/issues/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 commented 6 years ago

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 commented 6 years ago

@madmax983 i opened a new PR to build upon the POC you had working https://github.com/madmax983/lightning-redux/pull/15

madmax983 commented 6 years ago

@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 commented 6 years ago

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
maniax89 commented 6 years ago

@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 commented 6 years ago

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