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

waitFor leads to wrong design #209

Closed
tomkis opened this Issue May 19, 2015 · 9 comments

Comments

Projects
None yet
5 participants
@tomkis

tomkis commented May 19, 2015

I don't really think it has any benefit to keep waitFor method in dispatcher as it's leading to wrong design.

A great example of that is implementation in flux-chat. I was actually thinking about sending a PR where waitFor method is not used, then I realised that the only correct way would be to get rid of ThreadStore and UnreadThreadStore.

I believe the right approach would be to to keep entire state in one store and get rid of direct dependencies which may eventually lead to inconsistent state as the application grows. For example, once new action which modifies currentID is presented and handled only in ThreadStore.

If we need a state variable across multiple stores, it's much better to listen to all actions which modify the state variable instead of presenting dependencies between stores. waitFor does not have any other utilisation than waiting for stores to handle actions so that we can be sure that the state has been set already and we don't need to do that as long as we keep the variable in every single store that needs the variable.

@fisherwebdev

This comment has been minimized.

Contributor

fisherwebdev commented May 20, 2015

Are you suggesting that you would duplicate the logic and the cached value in multiple stores? That sounds like a maintenance nightmare. I vastly prefer a single source of truth.

Even if you have a global cache, with something like baobab, you would still potentially have one mutation that needs to maybe happen first, depending on logic, and then a second mutation that happens based on that value.

So we have something like this:

// StoreA, within the switch:
case FOO:
  if (shouldBarBeMutated()) {
    mutateBar();
    emitChange();
  }
  break;

// StoreB, within the switch
case FOO:
  waitFor([StoreA.getDispatchToken]);
  mutateBaz(StoreA.getBar());
  emitChange();
  break;

Not sure how you'd pull that off without waitFor() without duplicating logic and/or cached values.

@tomkis

This comment has been minimized.

tomkis commented May 20, 2015

Yes, I am suggesting to duplicate state. It does not necessarily mean that I need to duplicate logic. For the shared logic we can use stateless service (in Domain Driven Design terms). It's essential to realize the frequency of such a case (state dependency between stores). It's definitely a very minor use case and therefore I can imagine using shared stateless service as an ideal solution.

However, what does concern me a lot are tight dependencies between stores. For example the snippet you have posted above, let's imagine the application grows and there is a new action (qux) which mutates bar

// StoreA, withing the switch:
case QUX:
  mutateBar();

bar is correctly mutated and therefore consistent, however in StoreB baz depends on bar which has changed while handling QUX and therefore baz is not consistent. One solution would be to listen on the same action QUX even in StoreB which I found very complicated and error-prone.

@fisherwebdev

This comment has been minimized.

Contributor

fisherwebdev commented May 20, 2015

Yes, responding to the same action in both stores is how we handle this at Facebook.

The only problem we have with this approach is that an engineer may not realize they need to respond to a particular action in a "downstream" store. We've talked about and experimented with developing warnings whenever an upstream store changes in an action that is not handled by its downstream stores.

If you have a solution where the state and logic are not duplicated, we could consider suggesting this in the documentation. I'll take a look at stateless services.

@luisherranz

This comment has been minimized.

luisherranz commented May 20, 2015

Uhm... I'm going to jump in because this is interesting.

I've been using Flux in Meteor and after a while I realised I wasn't using waitFor at all. That's because in Meteor you can use reactive variables like this:

// StoreA:
var bar = new ReactiveVar("default value");
StoreA.getBar = function(){
  return bar;
}
// ...
// then, within the switch
case FOO:
  if (shouldBarBeMutated()) {
    mutateBar();
    // you don't need emitChange with Meteor's reactivity either
  }
  break;

// StoreB, not in the switch
Tracker.autorun(function(){
  mutateBaz(StoreA.getBar());
  // not need for emitChange either, UI's update reactively
});

StoreA.getBar() returns a reactive variable, bar, so Tracker.autorun will rerun again automatically whenever bar changes, keeping everything in sync.

Actually, when it comes to case QUX, it still works without any other code in StoreB:

// StoreA, withing the switch:
case QUX:
  mutateBar();

Because again, whenever bar changes, no matter where, all Tracker.autorun functions depending on bar rerun automatically again.

On other side, Optimizely's Flux implementation don't use waitFor either, because they use getters outside stores (point 3 of how Nuclear differs from Flux). They can do that because state is not spread out through stores, but contained in a single immutable map.

@fisherwebdev

This comment has been minimized.

Contributor

fisherwebdev commented May 20, 2015

Thanks for those explanations, Luis. I think this is somewhat similar to where a few folks I've talked to are going with Flux + Baobab. I'm very interested in the idea of a global state cache, accessible by all stores.

It does seems to reduce the amount of dependency logic, but I'm a bit concerned about the amount of hidden magic going on in a system where updates cascade through the data layer. This is exactly the situation that Flux was created to avoid. Making the updates explicitly declared in the stores helped us to reason about complex updates that touched a lot of domains. When mutations are happening reactively (I might say magically) it becomes more difficult to understand how data is changing throughout the application.

But perhaps it doesn't matter. Maybe all we need is to focus on writing in an FRP style throughout the stores, and I might be able to get on board with this. I'll take a deeper look at the suggestions you've offered.

@tomkis

This comment has been minimized.

tomkis commented May 21, 2015

Yeah, I definitely think that reasonable option would be to keep the store stateless and pull the state to single shared storage. The storage of shared state may be for example Baobab or any other immutable tree data structure.

That would mean stores would become just a dummy action handlers (business logic handlers) and I am still thinking about the name, as Store is probably not very descriptive in this case.

Bill, you have mentioned that even in this case, you most likely won't avoid the need for waitFor because you probably want to execute those dummy handlers in particular order as one handler may depend on state which has been reduced by other handler. Just please, keep in mind that waitFor will not help you when using Baobab. The reason why is because change is emitted only once on the end of the call stack (setting value to baobab and then immediately reading it, will result in reading old value).

Anyway, I would say that the need to use waitFor to keep particular order of handled actions is most likely caused by wrong architectural decision. Stateful store is supposed to encapsulate specific domain and the state is responsible to determine those boundaries. However, if the store is stateless and entire state of app is held in single tree, the decision about store boundaries is not that strict and we create those boundaries only to make our code logically grouped together (we could possibly have just single store to handle all actions, which is of course not ideal). The tree itself is responsible for emitting change for specific views which really need to update. So in other words, if we really need to rely on order then it's probably better to handle everything in single action handler, but in most cases we could avoid it by proper schema of the tree.

Even though Baobab is a great idea, there is in my opinion one even better approach. We can possibly have just one emit event for entire tree (any change in the tree would result in the emit change) and the topmost view would listen to the change and pass the state down through the component hierarchy via props. However, there is one requirement. All components but the root must be pure. If the condition is met and we are using immutable data structure (which we should anyway) we could possibly have PureRenderMixin in all our components and the immutable data tree with the mixin allows us to update views only when necessary. In that case we don't even need Baobab.

@alexeyraspopov

This comment has been minimized.

Contributor

alexeyraspopov commented Jul 23, 2015

There is my contribution on this topic.

You can treat Dispatcher's API as a low-level and create your definition of stores above. For example, in my Stores implementation I've used waitFor under the hood, inside state getter.

That means that you can just use getter and don't even think about waitFor. Check example here.

@ghost

This comment has been minimized.

ghost commented Aug 5, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@kyldvs kyldvs added the question label Nov 18, 2015

@kyldvs

This comment has been minimized.

Contributor

kyldvs commented Dec 16, 2016

I think at this point Redux captures the wait-for-less use case very cleanly. I would highly recommend that library. We will not be removing waitFor from our implementation of dispatcher any time soon though.

@kyldvs kyldvs closed this Dec 16, 2016

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