Cross store actions #201
Cross store actions #201
Conversation
|
@anacierdem What do you think? |
|
|
||
| const noop = () => () => {}; | ||
|
|
||
| export default class Container extends Component { |
There was a problem hiding this comment.
This will happen in another PR, just wanted to have a cleaner file and care about only one implementation
|
I will have a look 👍🏼 |
|
Being able to communicate with other stores is a very important moment 👍 I was always "OK" to do that outside of sweet-state to follow a sandwich pattern - My concern here is a limitation of "global->global" or "contained -> contained" What about onboarding some principles from "friend classes"(C++) or nestjs @module - basically an annotation that |
|
Considering the use cases in #146, this change would only cover a very limited portion of it. Let me try to list the useful use-cases and then let's think about each of them based on the proposed API.
The new addition does not help with this. Still I need to use container props.
This can be somewhat achieved if you move both stores under the same sharable container. That means the data boundary should be the same for both stores as an additional restriction. This may not always be meaningful. The ability to dispatch actions on other stores is definitely an improvement, but the limitations are a bit too strict.
I still need to pass in the external stuff from container props at the place where I render the container. Note that this is an important constraint when deciding to use a global/local container and when deciding where to place the container. When combined with the global/containedBy restriction for the new API, there would still be cases where it is impossible to dispatch to another store because of the container formation. This also generally requires keeping a copy of the state (#131) and this will still hold. So overall I think the ability to dispatch actions on other stores is useful but will solve a very limited set of problems especially with the restriction imposed. Also I think we should keep the other requirements in mind when searching for a solution even though we don't need to solve all at once in a single PR. P.S This also looks like an official solution to #139 |
I see the point but how would you protect from inadvertently getting data / triggering actions on the wrong store? <App>
<AllObjectsContainer>
<AllObjects />
</AllObjectsContainer>
<SelectedObjectsContainer>
<SelectedObjects>
</SelectedObjectsContainer>How do we tell that the developer intended to have
My answer for this was either push or pull. Means either |
I think the container rules should apply, for the above example those stores should not be able to talk to each other. It does not make sense to dispatch an <SelectedObjectsContainer>
<SelectedObjects>
</SelectedObjectsContainer>It wouldn't be much different than regular RSS usage. The same rules when enforcing a container if
Yes, which is already the case in the current state. You forget a container, and you end up on a global store. I think this is still inline with how we think container boundaries are set-up per-store when declaring the store. Now thinking about it more, having the new api in I think this would be a better restriction: "whenever targeted store's For example: When they are contained in their own boundary, these cannot talk: <AllObjectsContainer>
<AllObjects />
</AllObjectsContainer>
<SelectedObjectsContainer>
<SelectedObjects>
</SelectedObjectsContainer>These can: <SelectedObjectsContainer>
<AllObjectsContainer>
<AllObjects />
<SelectedObjects>
</AllObjectsContainer>
</SelectedObjectsContainer>Following along;
I am pretty sure there are some edge cases, but should not be very difficult to list all possible cases. Would this be a viable strategy? So basically, you should be |
|
Ok, I have updated the implementation, making it work in a more general way. I'll rase a PR to refactor some of the internals before merging this, so it is easier to grasp.
In reality the limitation with the current reworked implementation is: As either the validation is done at container level (old implementation) or at consumer/hook level (new one). I guess the hook level provides more flexibility and it is more similar to the hook composition pattern anyway.
Cannot think of any better API. We could enforce a whitelist on store creation, by requiring a const MainStore = createStore({
// ...
linkedTo: [OtherStore],
});And so if some action calls |
💯 With the boundary validation (containers throwing when an invalid consumer tries to access a store), I think accesses are pretty safe, but if we really want to we can introduce the I haven't looked at the implementation yet. I will do that soon as well. Thank you for the immense effort you are putting into this @albertogasparin. |
|
so let's talk in abstractions. As I understand two containers are allowed to communicate if the coexists at the whole continuity of their existence 🤓
So two questions to answer:
So let's try to derive simple rule:
|
|
When I started thinking about this, that was exactly what I thought @theKashey but as I think about it more, the order of containers started to look like it should not be important. I remember rendering the same container with the same scope twice at two positions to solve this issue before (item 2 in #146). With the new shared container, this is probably not an issue but still someone may want to keep separate containers while communicating between stores. I think that would be an artificial limitation (unless there is some implementation detail that prevents this for some reason). So in the light of this position;
In this case (where store A is contained by StoreAContainer), The global store's actions should be able to talk with the local store A instance if they are used inside
In the following scenario; Store B's action in the child component can definitely access store A. Similarly, store A's action should be able to access store B if dispatched in the child component as target store's (B) container is fulfilled.
I am not exactly sure why the order of the boundaries should matter? As long as the dispatching location fulfills the container requirement, it should be fine. If I can use both A & Bs hooks in the above child component, there is no reason why those two stores cannot talk to each other. So effectively, this is equivalent from child component PoV: So if we are introducing this as a native API, I think we should make it as smooth as possible. OTOH, let me know if this has a very bad unwanted consequence that I cannot see. |
|
@albertogasparin back to the data sharing:
That wouldn't need to keep an additional state in the listening store either as it is directly bound to another RSS state already. When it is through the container props OTOH, there is no guarantee around this and it could change irrespectively via other mechanisms, thus you'd need to keep a "previous" state to re-initiate an action on the listening store when the prop changes. When listening to another store OTOH, the only way that state changes unexpectedly (at least according to my current definition) is changing the container structure that is affecting the listening consumer. In that case, if the underlying mechanism re-calls the listener with the new container's value (as the old value is not valid anymore), there should still no need to keep an additional state. We can even instead do the same-value optimization and don't call the listener in the library. |
|
I am a little worried about allowing more than we should. Unidirectional data flow got adopted for a good reason - less 🎉 SURPRICES 😀 at work. So let's put aside "why" one need to dispatch actions from one store to another and focus on another important aspect - observability. Ie how to keep things as they are today:
In a little more abstract theory, two entities communicate via explicit "channels", aka wires between two stores. In/Out channels are more usually just separate API into a few different buckets facing different "types" of consumers. In port and adapters (and XState) that "wire" is a separate entity creating a connection between two entities not aware of each other. As example we can use today:
const someStateActions = useSomeStateActions();
return <AnotherStateContainer externalActions={someStateActions}>...
The essence of everything above - how to create explicit hardwiring between stores, like these are stores I need to talk(read data from or use in other way, they should be defined "among me"), and I am not going to use any other store. In other words - let's stick to the Tangerine goals and keep drawing artificial boundaries. |
|
First of all even with the boundary order restriction, I think enabling communication would be a great addition. Also I totally agree on the fact that the API should be designed such that the data flow is clearly and easily visible with a very explicit definition to prevent any surprises. Now let me think out loud: Today we lean on the data flow model of React by delegating the communication to jsx. This is not necessarily a bad thing, it is well known and predictable. OTOH, we all agree that jsx/React world is not the best place to do this communication and we are looking into ways in the RSS API itself. This means we are not subject to the unidirectional flow in the React tree. This does not mean we shouldn't have a regulated data flow though. With RSS, it is already possible to communicate between React tree nodes irrespective of the unidirectional data flow. Global stores can talk to the same state container irrespective of where they are in the tree, similarly scoped containers allow such a mechanism as well. I don't think there is a reason to prevent a similar communication channel between two different stores as long as we can find a way to declare this. Essentially we are creating independent stores to encapsulate their state but at the same time we want them to work together as if they are the parts of a bigger machine. If we are strict on the boundary order, there are a few ways how an A action can dispatch some action in B when the natural container order is in reverse; Via an artificial scopeThis works in practice, I don't exactly remember if there were any side-effects. But clearly this is not something you would want. This fact forces you to reverse the container order just to make this more comfortable. <StoreAContainer scope='scope-to-inject-B-actions'>
<OtherthingsUsingStoreA/>
<StoreBContainer>
<StoreBConsumer>{
(_, {actionInB}) => {
<StoreAContainer scope='scope-to-inject-B-actions' externalAction={actionInB}>
<ChildComponent/>
</StoreAContainer>
}
}</StoreBConsumer>
</StoreBContainer>
</StoreAContainer>Through jsxThis means the relation between the action in store A and B is lost in a huge pile of jsx unless you know where to look at. This is a good example how you can loose observability in your analogy. <StoreAContainer>
<OtherthingsUsingStoreA/>
<StoreBContainer>
<StoreBConsumer>{
(_, {actionInB}) => {
<ChildComponent onActionAComplete={actionInB()}/>
}
}</StoreBConsumer>
</StoreBContainer>
</StoreAContainer>With a refI actually never tried this method. It will highly likely work as expected but it is not very pretty. It is highly likely that you'll need to drill down the ref when const actionInB = useRef();
<StoreAContainer externalAction={actionInB.current}>
<OtherthingsUsingStoreA/>
<StoreBContainer>
<StoreBConsumer>{
(_, {actionInB}) => {
actionInB.current = actionInB
}
}</StoreBConsumer>
</StoreBContainer>
</StoreAContainer>This is why I think we should at least consider relaxing the constraint. Now how should we declare the wiring between them is another topic. Let me brainstorm assuming we allow two way communication given relaxed boundaries; // Here store A is clearly importing from another module and the relation is clear
import StoreB, { actions as actionsB } from '../store-b';
const actions = {
actionA: () => ({dispatchTo}) => { dispatchTo(StoreB, actionsB.do()); }
};
const storeA = createStore({
initialState,
actions,
containedBy: containerA
})So in general one can analyze who is using a store's actions (thus depending on it) irrespective of the React tree shape and this looks pretty much clear to me at this point. In this example, StoreA depends on StoreB, meaning any use of it should also be contained by an allowed container of B. So in reality it is not the React tree that defines the relation but the store definition itself. React tree is a constraint that follows the store implementation. Another potential edge case is A importing B and B importing A causing a circular dependency but this is not something we can solve with React tree restrictions or RSS API anyways. It is a programming reality 😄 |
|
So let's pick the first thing to agree on:
Reatom for example solves this from an interesting point of view - export const onSubmit = action((ctx) => {
const a = ctx.get(atomA);
const b = ctx.get(atomB);
ctx.set(atomC, a + b);
// dispatchTo(storeC, otherActions.update())
}, 'onSubmit')So what about adding |
|
I call these the "units" of a state management system:
All these can freely import and use other parts of the state tree and communicate via direct dependencies and this enables something very important IMO. You can design "flow"s that describe business logic that span multiple concerns. I am totally onboard with an |
That is why I was suggesting something like a
Happy to, I think the const actions = {
actionA: () => ({dispatchTo}) => {
const allData = dispatchTo(AllObjects, actionsAll.getData());
...
}
};but having AllObjects pushing data to SelectedObjects is complicated:
Having hard time to map to an API as while for an action we have a clear origin (either consumer hook or container), the subscription seems a bit more iffy. const SelectedObjects = createStore({
initialState,
actions,
handlers: {
onInit: () => ({ listenTo, setState }) => {
listenTo(AllObjects, (newState) => {
// how do we make newState available? If we have to put it in state, is it really an improvement?
setState({ allData: newState });
});
}
})But that will not make "compound" data handling much easier, as SelectedObjects might need to store AllObjects state in other places like selectors. At that point container props behaviour is a bit nicer as at least they will be available in all actions by default 🤔 |
albertogasparin
force-pushed
the
feature/cross-store-action
branch
from
e74c598
to
1f0cb01
Compare
|
You've opened a pandora box, basically You Might Not Need an Effect, or
The first one is easy to implement, easy to use, easy everything including easy to make a mess. There is a long history between each concept with wars, wins and losses. There are good use cases for each one, and maybe one day we will have them both. |
I think it should be the container. While thinking about
From the PoV of the "provider" store, selectors would behave exactly the same. They only trigger the consuming store if the output of the selector is not shallow equal. That would mean we need to find an API to reference the selector in the consumer, that's true. In #146, we use plain react hooks from
Is there really a need to unsubscribe? I never had that requirement, I think we can simply postpone this extension.
Not much of an improvement, I'd agree. That's why we have to use a const SelectedObjects = createStore({
initialState,
actions,
handlers: {
onInit: () => ({ listenTo, dispatchTo }) => {
listenTo({
store: AllObjects,
selector: allObjectsSelectors.idSelector,
}, (newState) => {
// In general, we want to "react" to the changes in the other store. If listenTo only triggers when
// the output of the selector changes, we don't have to keep it in our state, we can just use it in
// the action as a bound parameter on the stack.
actions.updateSelected(newState);
// If I really want to access a value from the other store:
const data = await dispatchTo(AllObjects, allObjectsActions.getData());
});
}
})
That's exactly what we do in #146 and in the above example as well. Whether it is safe or not is another question, that's true, but the only alternative right now is already using an effect in React world with the existing API.
I am not sure I get this one @theKashey, can you provide an example?
This, I agree but then this is only a partial solution to the store communication problem. If this is something we should not rush, what do you think of the With the new setup though, I think |
Hmm, I think it would look something like this if I'm getting it right; const SelectedObjects = createStore({
initialState,
actions,
handlers: {
onInit: () => ({ dispatchTo }) => {
// listenToX simply sets a callback to AllObjects' state that it will call it when it sees necessary
dispatchTo(AllObjects, allObjectsActions.listenToX((newState) => {
actions.updateSelected(newState);
}))
}
})I think we already have most of |
|
Let's look on the problem from coupling point of view
const SelectedObjects = createStore({
initialState,
actions,
handlers: {
onInit: () => ({ listenTo }) => {
listenTo({
store: AllObjects,
selector: myAllObjectsSelectors.idSelector,
}, (newState) => {
actions.updateSelected(newState);
});
}
})
const AllObjects = createStore({
initialState,
actions: {
//...
someAction: ({setState, emit}) => {
setState({thatThing:1});
emit('to-whom-it-may-concern');
}
}
handlers: {
onChange: (diff, {emit}) => {
if(someSelector(diff.before, diff.after)) {
emit('some-change', diff.after);
}
}
})
const SelectedObjects = createStore({
initialState,
actions,
handlers: {
onInit: () => ({ on }) => {
on({
store: AllObjects,
event: 'some-change',
}, (newState) => {
actions.updateSelected(newState);
});
}
})I really don't like the second example, even if the majority of event-driven system looks like it.Looking at the code samples I could say:
|
Expose a new API as part of
StoreActionsApicalleddispatchTo:Limitations
To reduce chances of unexpected behaviour due to stores container instances, it will only work in 2 cases:
containedByvalue is the same on both dispatching store and targeted storeIn any other case it will throw a runtime error: a bit harsh but otherwise a different store might be created/affected and that would make debugging the problem harder.