fdecampredon / rx-flux

The Flux architecture with RxJS
255 stars 15 forks source link

Discussion: The right API for rx-flux #10

Open fdecampredon opened 9 years ago

fdecampredon commented 9 years ago

The Flux pattern is really interesting because in its essence it is fully reactive. I think it perfectly matches the Rx way of doing things and combined with rx-react, rx-flux could result in a solid architecture.

However, I have some problems to figure the right API for rx-flux. I want to publish this project on npm but, before that, I want to create an API that will provide this project users simplicity, elegance, and freedom.

A good library is hard to design. Despite the fact that it is its role to constraint the user to develop in a certain way, to reduce boilerplate and transform the hard into simple, each feature that you offer makes that library less usable for people who don't want to use those features. Sometimes it's better to let the user build their own abstractions on top of your library than trying to solve every use case.

The Store

The only thing I'm pretty happy with actually in rx-flux is the store object. When you think about what is a Store in the Flux pattern, the answer is pretty simple : a reduce operation. At a given instant, the value held by the store is the result of a reduce operation over the list of all actions that have been dispatched. That perspective make things easy to reason about. For example, if your store state is erroneous it's easy to understand where the problem comes from by recording/replaying action. rx-flux stores mimics exactly that model: a seed value and a set of transformations for each action dispatched.

var TodoStore = Store.create({
  getInitalValue: () => ({}),
  getOperations: () => 
    Rx.Observable.merge(
      todoCreated.map(todo => ({ 
        transform: (todos) =>  ({ ...todos, [todo.id]: todo })  
      })),
      ....
    )
}

Singleton Dispatcher vs Independent Actions

I tried to reduce all the boilerplate code of the original Flux pattern by creating function-object ala reflux and getting rid of the central dispatcher. After experimenting a bit with it, I dislike the result.

An rx-flux action has too much responsibility; it is the constant, the action creator and the dispatcher at the same time. Worse, that pattern does not provide the single source of truth that is the central dispatcher in the original Flux pattern.

Unless someone completely disagrees and proves that I should not do so, I'm seriously considering getting rid of those function-object \ and reintroducing a global singleton dispatcher. Sure it is a little more verbose, but the single fact that you can record/replay all the actions from a single source worth the deal in my opinion.

The ActionCreator problem

There is something that I dislike and want to replace in the original Flux pattern: ActionCreator.
In my point of view, ActionCreator is the garbage of the original Flux pattern where everything that breaks the unidirectional data flow has been put. An ActionCreator validates the input and creates the action object - no surprise here - but it also dispatches the action and acts as command by performing an async task and dispatching another action depending on the result.
Now I still think that there is a need of a simple function that creates an action and validates the input but it should not dispatch the created action itself nor it should perform any other tasks. Instead, I want to let the component dispatch itself the value returned by the action creator and let the WebService layer listens dispatched actions and performs async tasks.

The case of async task

So here is the problem that I want to solve from the start and that gives me so much headache: When an action is associated to an async task, if that task fails, the store may have to revert its state.

Simple case are obvious but this can create a lot of complexity. For that purpose, an operation consumed by an rx-flux store can provide a promise as a confirmation token :

todoCreated.map(todo => ({ 
  transform: (todos) =>  ({ ...todos, [todo.id]: todo }),
  confirm: aPromise
}))

When the promise is resolved, the operation is confirmed and in the contrary, if the promise is rejected, the store state is reverted.

Now the problem is: how we generate that promise.

In the current rx-flux version, I have something similar to the ActionCreator so it is more or less easy. The ActionCreator performs the async task and the promise is just a property of the action object. However, that strategy causes more problems that it solves. Firstly, as I said, I don't want the ActionCreator to be responsible of executing the async task. Also, I think that the action object should be a serializable object. Finally some stores can be interested in that async task failure/success so the result of that task should also dispatch an action.

The original Flux pattern promotes 3 constants per action. The first one for the original action, one for failure and one for success. This is a bit verbose but more importantly, it does not provide a way to see how the result/error actions are related to the original one. Reflux has some sort of async actions but does not really promote a way of resolving things.

Proposals

Here are the different proposals I have thought about. For each one of this, I'll show a little example of a todo creation.

Simple Dispatcher with getAsyncToken

In this proposal, each action has an unique id generated by the Dispatcher; that helps creating the relation between success/failure but it produces a lot of boilerplate code.

// AppDispatcher.js

// The app dispatcher pretty similar to the original flux one, 
// except the registration/listening process That is based on rxjs
var Dispatcher = require('rx-flux').Dispatcher;

module.exports = Dispatcher.create(); 

//TodoConstants.js
var keyMirror = require('keymirror');

//Three constant by *async* action, one for the action one for failure one for success
module.exports = keyMirror({
  TODO_CREATE: null,
  TODO_CREATE_SUCCESS: null,
  TODO_CREATE_FAILURE: null
});

//TodoActions.js

var TodoContants = require('./TodoContants');
var uuid = require('uuid');

// the action creator, responsible for creating the action object and validating the input
module.exports = {
  todoCreate(text) {
    if (!text) {
      throw new Error('text should not be empty');
    }
    return {
      type: TodoContants.TODO_CREATE,
      data: {
        id: uuid(),
        text: text,
        completed: false
      }
    }
  },

  todoCreateSuccess(action, result) {
    return {
      type: TodoContants.TODO_CREATE_SUCCESS,
      parentAction: action.id // the action is related to the original action through the *id*
      data: result
    }
  }

  todoCreateFailure(action, error) {
    return {
      type: TodoContants.TODO_CREATE_FAILURE,
      parentAction: action.id // the action is related to the original avtion through the *actionid*
      data: error
    }
  }
}

//TodoCommands.js

var TodoConstants = require('./TodoConstants');
var TodoActions = require('./TodoActions');
var AppDispatcher = require('./AppDispatcher');
var WebApi = require(...);

// Commands are responsible of executing async task
AppDispatcher
  .getObservable(TodoContants.TODO_CREATE)
  .subscribe(function (action) {
    var todo = action.data;
    WebApi.createTodo(todo).then(function (result) {
      //on success we dispatch a *child action*
      AppDispatcher.dispatch(TodoActions.todoCreateSuccess(action, result))
    }, function (error) {
      //on failure we dispatch a *child action*
      AppDispatcher.dispatch(TodoActions.todoCreateFailure(action, error))
    })
  })

//TodoStore.js
var Store = require('rx-flux').Store;
var TodoConstants = require('./TodoConstants');
var AppDispatcher = require('./AppDispatcher');

var todoCreated = (
  AppDispatcher
    .getObservable(TodoContants.TODO_CREATE) // create an observable that filter by action type
    .map(todo => ({ 
      transform: (todos) =>  ({ ...todos, [todo.id]: todo }),
       // a promise resolved when a *child* action with the constants is dispatched
      confirm: AppDispatcher.getAsyncToken(  
        TodoContants.TODO_CREATE_SUCCESS, 
        TodoContants.TODO_CREATE_FAILURE, 
        action.id
      ) 
    })),
)

var TodoStore = Store.create({
  getInitalValue: () => ({ }),
  getOperations: () => Rx.Observable.merge(todoCreated)
}

//component dispatching

...
createTodo(event) {
  var text = event.target.value;
  AppDispatcher.dispatch(TodoActions.createTodo(text)); // the component is responsible for dispatching
}
...

Dispatcher with integrated command

Now in the second proposal, the Dispatcher provides a 'command' mechanism while this reduces boilerplate code and constraints the usage. I'm not so confident it provides the necessary freedom. For this proposal, I'll provide the Dispatcher api (typescript/flow notations).

interface Dispatcher {
  /**
   * Produce an observable that notify of dispatched action,
   * If type is provided action are filtered by this type
   */
  getObservable(type?: string): Rx.Observable 

  /**
   * Dispatch an action
   */
  dispatch(action: {type: string, data: any})

  /**
   * Register a command, a command is a function that return a Promise
   */
  registerCommand(type?: string, command: (action) => Promise<any>);

  /**
   * observable exposing 'command' results
   */
  commandResult(type: string): Rx.Observable
  /**
   * observable exposing 'command' errors
   */
  commandError(type: string): Rx.Observable;
  /**
   * observable exposing 'command' completion
   */
  commandComplete(type: string): Rx.Observable;
  /**
   * observable exposing async task status
   */
  commandStatus(type: string): Rx.Observable<boolean>;

  /**
   * return a promise that resolve with the result of commands associated to the given action type
   */
   getAsyncToken(action: { type: data}): Promise
}

```javascript
// AppDispatcher.js

// The app dispatcher pretty similar to the original flux one, 
//  except the registration/listening process That is based on rxjs 
// and the provided command system

var Dispatcher = require('rx-flux').Dispatcher;

module.exports = Dispatcher.create(); 

//TodoConstants.js
var keyMirror = require('keymirror');

// only one constant for async action, other constant are generated by the Dispatcher,
// in this case TODO_CREATE:result TODO_CREATE:error
module.exports = keyMirror({
  TODO_CREATE: null
});

//TodoActions.js

var TodoConstants = require('./TodoConstants');
var uuid = require('uuid');

// the action creator, responsible for creating the action object and validating the input
module.exports = {
  todoCreate(text) {
    if (!text) {
      throw new Error('text should not be empty');
    }
    return {
      type: TodoConstants.TODO_CREATE,
      data: {
        id: uuid(),
        text: text,
        completed: false
      }
    }
  }
}

//TodoCommands.js

var TodoConstants = require('./TodoConstants');
var AppDispatcher = require('./AppDispatcher');
var WebApi = require(...);

// Here we simply call the WebApi.createTodo that return a promise
// Success/failure action are automaticly created/dispatched
AppDispatcher.registerCommand(TodoConstants.TODO_CREATE, function ({data: todo}) {
  return WebApi.createTodo(todo)
})

//TodoStore.js
var Store = require('rx-flux').Store;
var TodoConstants = require('./TodoConstants');
var AppDispatcher = require('./AppDispatcher');

var todoCreated = (
  AppDispatcher
    .getObservable(TodoContants.TODO_CREATE) // creates an observable filtered by action type
    .map(todo => ({ 
      transform: (todos) =>  ({ ...todos, [todo.id]: todo }),
      confirm: AppDispatcher.getAsyncToken(action)  // a promise that resolves to whatever the command associated to the action resolves
    })),
)

var TodoStore = Store.create({
  getInitalValue: () => ({}),
  getOperations: () => Rx.Observable.merge(todoCreated)
}

//component dispatching

...
createTodo(event) {
  var text = event.target.value;
  AppDispatcher.dispatch(TodoActions.createTodo(text)); // the component is responsible for dispatching
}
...

Conclusion

Flux is a solid pattern that makes our application state easy to reason about. So before publishing rx-flux, I really want to grasp the right api. Any proposal/comment/opinion would be greatly appreciated.

schrepfler commented 9 years ago

Would it be possible to design the API so that one can use cycle.js rather than react? https://github.com/staltz/cycle

fdecampredon commented 9 years ago

@schrepfler rx-flux does not have any dependencies except rx, so I guess you could use it with any framework/lib, but I won't introduce any feature just for the goal of cycle.

donaldpipowitch commented 9 years ago

I started to learn React and RxJS and of course I searched for a project which uses both like this one, but recently I think a project like this overcomplicates the API. I would prefer a project which aggregates best practices for plain RxJS with plain React (and maybe something like immutable.js). Without an additional library one could even easily switch from RxJS to Bacon.js or Kefir and from React to virtual-dom.

donaldpipowitch commented 9 years ago

I think this is a good example for why this framework make things more complicated instead of easier (at least for me). From the docs: "An action cannot propagate an error or complete notification, if an error is thrown in the map function that error won't be catched :". Why introduce something like this in the first place?

fdecampredon commented 9 years ago

Hey @donaldpipowitch, thank you for your feedbacks.

I started to learn React and RxJS and of course I searched for a project which uses both like this one, but recently I think a project like this overcomplicates the API. I would prefer a project which aggregates best practices for plain RxJS with plain React (and maybe something like immutable.js).

I can perfectly understand your point of view, however I argue than some React construct make it hard to be directly consumed in an rx way, lifecycle and events handler for example are very imperative and without some helpers it will be hard to use it with rx without verbose and over complex constructs.

Without an additional library one could even easily switch from RxJS to Bacon.js or Kefir and from React to virtual-dom.

Honestly I don't see the point here, react/v-dom, bacon/kefir/rxjs, this two set of library address more or less the same problem but in different way. If I build a bridge/tools for one pair, I don't see how you can expect it to be compatible with the other ...

Now I think your comments is more about rx-react than rx-flux, and you should perhaps give a look at this project. (while related they are both independent).

I think this is a good example for why this framework make things more complicated instead of easier (at least for me). From the docs: "An action cannot propagate an error or complete notification, if an error is thrown in the map function that error won't be catched :". Why introduce something like this in the first place?

If you look carefully at the proposal of this issue it's especially something that I want to remove. I never published rx-flux especially because I never found the right api for it, if you have some good proposal I would be interested :).

donaldpipowitch commented 9 years ago

I can perfectly understand your point of view, however I argue than some React construct make it hard to be directly consumed in an rx way, lifecycle and events handler for example are very imperative and without some helpers it will be hard to use it with rx without verbose and over complex constructs.

That's probably I didn't notice yet because of my lack of experience and it is definitely good to solve this.

And sorry for the confusion. I indeed mixed up rx-flux and rx-react!

arqex commented 9 years ago

Ey @fdecampredon

I think you should reconsider ditching the dispatcher in favor of using events to dispatch your actions:

http://arqex.com/1028/better-flux-dom-events

Since your approach uses Rx to create your actions & stores, what better than consume component events directly?

Cheers!

fdecampredon commented 9 years ago

That's interesting, but create some problems, like server-side rendering, need to think a bit more about it, thanks for the proposal!

arqex commented 9 years ago

The actions in the components is an issue related rather to the user interaction than to the rendering.

You usually don't need to listen to component dispatched actions to render your page, but in the rare case you do it in the server, you can create a similar solution using EventEmitter. The key is not to add a propietary piece of code inside the views ( components ) because that would couple them to your library.

In your library stores react to action events and the view react to store events. Why don't to make actions react to views events too?

BerkeleyTrue commented 9 years ago

Actions are already reacting to the view. You can create an Action called onClickSend, in your view you can tie a button to the action like so

\\ in Rx-Flux Store definition
getOperations: ()=>
  return actions
    .onClickSend
    .map(()=>
      /* do magic */
    );
\\ in React render function
<button onClick={ actions.onClickSend }> Send</button>

Now your action is listening to your view, your store is listening to your action, and the view is listening to your store.

BerkeleyTrue commented 9 years ago

@fdecampredon I am also not in favor of introducing a dispatcher.

I think the current implementation is great and is very unopinionated. One of the points in the article above is that a lot of the flux libraries out there end up tying you to it. I think keeping this project as close to plain RxJS is a better approach.

What I would like to see a central flux object that can help out with things like (de)hydrate stores for isomorphic applications and a reduction in boilerplate, i.e. creating multiple actions at once using an array of action names.

arqex commented 9 years ago

@BerkeleyTrue

In your example the action is not listening to the view, the view is calling the action. That is an imperative way of programming instead of reactive and couples the view to the the action, if you delete the action you break the view.

BerkeleyTrue commented 9 years ago

@arqex Could would you demonstrate how someone could achieve this declaratively? In order to satisfy your requirements, views would need to expose observables, actions subscribe to those observables, and stores subscribe to actions.

At this point, why are actions necessary? They just become a relay. Then the store listens to the view's observables, which are now looking really similar to rx-flux actions.

In in the article you linked above seems to be using the same method. I am really curious as to how this would be accomplished declaratively, especially with the button example I gave above.

arqex commented 9 years ago

From my point of view, actions is where the app logic should be, there your fetch data from the server and coordinate how to update the store, so I like them to stay ( save the (re)actions! ).

I'd like to have views completely decoupled from any framework, so I wouldn't make them expose observables either. Since I can't use custom react.js synthetic events to communicate (that I think would be the perfect solution), the views just dispatch common events like if they were HTML elements.

You can create the observable at the document object, and actions would subscribe to it instead make the view know about observables.

Emit an event is an imperative action. Maybe I mixed up sematics imperative/declarative, ?/reactive. If we want to make it more declarative we could create some mixin to do something like:

// In your component render
<button onClick={ this.thenTrigger('form:send') }>Send</button>

The difference of this code with the one you wrote is that this code doesn't call an action, it just notify that the form wanted to be sent. It is reactive because is up to the action to react to the event, instead of being called by the view.

BerkeleyTrue commented 9 years ago

@arqex That is a good point. Would there be an issue with circular dependencies (views require actions require stores require views) ?

arqex commented 9 years ago

That's the strongest point of reactivity, there are no dependencies at all, since your actions stores and views don't use the others, they just emit events. You can unit testing them separately or reuse your views in other projects with no modification.

BerkeleyTrue commented 9 years ago

But they still need to subscribe to each other. Using commonjs you would then need each script to require the other. Unless there is something I am missing.

TimothyKrell commented 9 years ago

@arqex If you have the actions listening to the views, it seems that you would have to have one action merging events for many views if you want to be able to do the same action in multiple places in your app. Wouldn't that start to clutter the action code, instead of having each component require and call the actions it needs?

arqex commented 9 years ago

Hi @TimothyKrell

You can have multiple actions listening to the same event, like if they were action callbacks in a traditional flux implementation.

TimothyKrell commented 9 years ago

@fdecampredon Have you thought about using contexts to pass down some kind of event emitter or observable that components can emit their events on? Then all actions could just subscribe to that observable. This would be very similar to what @arqex suggested but without DOM events. Users would be free to create whatever they wanted to add to the context and use in their views.

TimothyKrell commented 9 years ago

@fdecampredon I'm not sure if you are still into hearing more ideas or if you've already figured something out that you're happy with, but I just thought of something that seems like it has potential. I suggest using higher-order components to expose actions on the users components. For every set of actions (todo actions, message actions, ect.) the lib could create a higher-order component. The higher-order actions component would expose functions as props on the users component. The action components would expose observables which the actions could map over.

The upshot of this is that actions only ever need to observe one component, the action higher-order component.

I need to think about this more, but I might use this in my own project. It could be more unnecessary hoop-jumping, though. Just thought I'd throw it out there!

fdecampredon commented 9 years ago

that's an interesting idea @TimothyKrell I need to think about it, I'll start soon a new app based on react, I plan to finish the first release of this repo at the same time, since using it with a real app can be a pretty good exercise to see good/bad point of an api.

TimothyKrell commented 9 years ago

Sounds great! :+1:

nmn commented 9 years ago

I have one major concern about not having a dispatcher and using RxJs at all. This is not specific to Rx-Flux, and most Flux implementations are ignoring this problem:

the flow of flux is: Action -> (Dispatcher ?) -> Store -> View

This works well enough, and the dispatcher is pretty useless for simple use-cases. The problem is really when dealing with Async actions.

Take this case: A simple app that displays a number with a + and - button next to it.

We want to send a request to a server everytime the number is changed by clicking a button. So when the user clicks the + button, we fire an 'INCREMENT' action. We also fire an AJAX request to the server with the new value for the number. The store updates immediately. When the AJAX request is completed, it ignores it if it succeeded, or rolls back if the request failed.

This is all pretty straightforward, except for the roll-backs. The roll-backs are made easy with Immutable data.

The question comes with how we deal with actions WHILE we are waiting for a response. If the user clicks + again while the AJAX call is pending, we would have increased the number by 2. If the first AJAX calls fails at this point we would go back to the original number. Now if the second AJAX call succeeds, we will either stay at 0 (if we ignore) or jump back to 2. The correct answer here is to be at 1.

This use-case is something almost everyone knows about, but most people are ignoring. I hope you take a crack at it.

BerkeleyTrue commented 9 years ago

@nmn Rx-flux has a concept of 'confirm' in operations. The behavior you describe can be done using a promise for each operation.

nmn commented 9 years ago

@BerkeleyTrue Thanks for the heads-up. Should've read the README more carefully.

With this in mind, I no longer think a central Dispatcher is important. It could probably be an optional piece of the library.

BerkeleyTrue commented 9 years ago

I don't think it's in the readme yet. The todoMVC example and the test are the best place to see the updated Store api.

The dispatcher usefulness in event driven flux implementations goes to almost nil. The only thing left that might be of value is the only one synchronous action at a time paradigm of flux. Reading over the flux documentation reveals that this is a key feature and I don't think a framework can call itself flux unless it can do this. Although I am still not convinced that this is something necessary in a framework, I don't think I am smarter/more experience than the whole of facebook's React/Flux team.

nmn commented 9 years ago

I've actually been confused about the only one synchronous action at a time.

In many simple implementations, this would be enforced anyway. For example, if using a simple event-emitter, anytime you fire an event, all listeners are called synchronously.

So, the first action would be dealt with, before you can even fire a second action.

The difference may come if the actions aren't truly synchronous. For example, if the listeners were only being fired on nextTick or something. I don't think that's the case with RxJS. So I still don't see the need for a dispatcher.

On the other hand, maybe what the paradigm means is one Action per tick of the event loop. This way, one action can be consumed fully and the view can be updated, before another action is dealt with. Somehow this doesn't sound right to me.

BerkeleyTrue commented 9 years ago

Yes with RxJS if you call an action it notifies its observers synchronously, so I guess either way it does not apply to RxJs versions of flux.

I think all that matters is that one action cannot fire until the previous action has completed notify it's observers or in the case of regular flux, calling all it's registered callback.

zoomclub commented 9 years ago

I've impressed with Rx now that I've looked at it further, in contrast to the different Flux implementations that are not so impressive. I'm all in favor of keeping to an uncluttered implementation of Rx and React and the idea from @TimothyKrell on March 16th aligns most with my thoughts on this matter.

Essentially, I just want React components to act as observables that capture specific events. Actually there would only need to be one core observable (similar to the context @TimothyKrell proposed) and it would be refed by multiple components. Each component can still specify the event types it would route to the observable, which would be possible using Rx Event Shortcuts as described here:

https://github.com/Reactive-Extensions/RxJS/blob/master/doc/gettingstarted/events.md

The core observable context can then be contained in a singleton object. I call this object the Interactor or Dispatch, it's role is to receive the input from all components via it's core observer and then to activate different actions. Actions are just related handlers in prototype objects (action-packs). Actions then process the event stream in the core observable further to produce the subsequent streams each action may require.

Key to this scenario is action activation, which is handled by the Interactor. At any given time there would be only one action active (possibly two if two components were engaged at once). The interactor would track what action is active, acting as a FSM for actions. Actions would just be plugins that know nothing about the view. They would simply use the core observable stream for their own purpose, so event streams are not routed to the specific active action, rather they are just the receiver if they are active.

Stores in my scenario are just Firebase node refs (as many as required with very little overhead entailed). Actions change the data at these ref points and ReactFire Mixin updates component state and the views take it away from there and render the updated state.

I'm just looking to have React be the view and Rx to be the interaction manager and Firebase to manage state and not complicate things with that Flux pattern. All this requires is the standard React component lifecycle functions and an output mixin for Rx and an input mixin for Firebase. These three foundation corner stones together with actions are then similar to the MOVE pattern as well:

https://cirw.in/blog/time-to-move-on

Such a foundation then makes it possible to focus on plugable actions and the meat of the app. Actions of course are much more than just calls to fetch data, there are actions dedicated to almost anything and they are smaller development tasks once the foundation is in place. I think it is best to do what is right and that likely means staying away from that Flux pattern as it just causes headaches :)

nmn commented 9 years ago

what I really appreciate about Rx-Flux is the focus on stores. Many existing flux libs make a robust dispatcher, but do the bare minimum for the store. Being able to use commit and reject for store data transformations is my favourite feature of RX-FLUX. There are still some problem that I'm having that are unsolved, but I can't think of a better way yet.