yahooarchive / dispatchr Goto Github PK

TL;DR If an error is thrown whilst handling an action the Dispatcher will not allow any further actions to be dispatched.

The logic that causes this is, I think, contained within Dispatcher::dispatch:

Dispatcher.prototype.dispatch = function dispatch(actionName, payload) {
    if (this.currentAction) {
        throw new Error('Cannot call dispatch while another dispatch is executing. Attempted to execute \'' + actionName + '\' but \'' + this.currentAction.name + '\' is already executing.');
    }
    ...
    this.currentAction = new Action(actionName, payload);
    ....
    this.currentAction.execute(handlerFns);
    debug('finished ' + this.currentAction.name);
    this.currentAction = null;

If an error is thrown during Action::execute, this.currentAction is never reset to null and so further calls to dispatch throw the 'Cannot call dispatch' error.

Using try around the Action::execute call prevents the 'Cannot call dispatch' error from being thrown under legitimate circumstances, in fact there is a test case ensuring that dispatch() does not swallow errors thrown by Store handlers.

Given that the current behaviour renders an app unusable, what are the suggestions for handling errors in a Fluxible app?

M

First off, thanks for open sourcing this! It has solved a number of problems I came across with the standard dispatcher as well as just making things much cleaner and easier to understand. That said, at this point the only way for me to use it, since I'm developing on top of rails and not node, is to copy the files over by hand into my source. It would be great if a bower.json file could be added and the code made workable for environments that don't use CommonJS. It seems like the two options are either a build task that converts the various libs into something consumable by the browser or making each file adaptable to either AMD, CommonJS, or the global object in the way EventEmitter does. Are either of these options feasible? I'd be happy to try and turn it into a PR if there's consensus on the way forward.

If an update to a Store creates a data condition that causes my react components to fail to render, the stacktrace is lost and does not make it to the error console.

I added the following to get me through but, I'm sure there's a cleaner way or maybe the developers have some further insight as to why this could be happening. But without the try/catch my react errors happen silently.

BaseStore.prototype.emitChange = function() {
    try {
        this.emit(CHANGE_EVENT, this.constructor);
    } catch (e) {
        console.error(e.stack);
    }
};

It seems that I can only mix in top-level properties. When I mix in Mixin with handlers, they are missing.

More information on Flux shows that Facebook is using the dispatchers completely synchronously and handling I/O in separate actionCreator functions that are called pre-dispatch.

We should evaluate the benefits of this over handling async actions inside of stores.

because currently module.exports = Dispatcher, it's not possible to make different dispatchers that register different stores in the same process. This is troublesome especially on the server side, where you might want to have multiple 'apps' running in the same process.

instead, if module.exports = function(){ var Dispatcher = function(){} /* etc etc */, return Dispatcher; } then one could make multiple dispatchers hooked up to different stores, depending on whatever.

Yes?

I'm using constants instead of strings for action names to have them all in a single place as a documentation. An additional benefit could be that if I make a typo in a name of a constant, thus passing undefined value, Dispatcher#dispatch could throw an error making it much easier to spot such typos.

What do you think?

This method would allow stores to control whether they should dehydrate themselves on the server to be sent to the client. We have run into times where a store may have listened for an action, but we did not need the store to be rehydrated on the client. For instance, one-time-use server-only data.

In dispatchr's dehydrate method:

• check if store implements shouldDehydrate method
  • if method exists, call it
    • if returns true, call dehydrate and return store's state
    • if returns false, ignore the store's state completely
  • if method does not exist:
    • always dehydrate (default to true)

With Facebook dispatcher, we can have a default: case in switch statement which will get executed for all the actions firing in the app. It is useful in the cases where developer wants do stating, logging etc. Even though this dispatcher makes registering process neat, we lose this ability which is useful in many cases

Dispatchr requires the node module util. Dispatchr seems to use only one function from util; inherits.

However, util doesn't actually implement inherits themselves. It simply re-exports the inherits node module.

So, why not avoid the middle man and use the inherits module instead? This will save us quite a few kilobytes.

It'd be nice if promises were baked into the API in such that any async operation could either return a promise or take a callback.

this could be just something really invalid, so consider this as a question if this is something that doesn't make sense to be implemented.

imagine i have a store, which holds a list of items. and i've an Item renderer component which renders each of these Items. Lets say each of the item component listens to store changes. with the current design, whenever one of the items changes in the store, all the instances of the item components will be re-renderered. assume if the store is able to attach an id in the emitChange call. so store says, i've a change, but only for item with id x. that would let the item renderer component to ignore the change notification if the change is not associated to it.

We should discuss the best way to abstract the /utils/BaseStore.js and /utils/createStore.js into a separate module.

This module requires a setImmediate shim to work in the browser and I didn't see it documented anywhere. Without debugging enabled in a webpack bundle, this was really annoying to track down. I think you should load the shim if needed at the top of the files it is used in.

Just wondering can the implementation of waitFor() handles circular dependencies, for example, StoreA waits for StoreB and vice versa?

Current the Dispatcher module exports a function that returns the Dispatcher instead of returning the dispatcher class directly.

I think this is bad because every time that function is called it creates and returns a new class instead of using the same one, new objects create by those different classes will point to a different prototype object with methods that have exactly the same code being duplicated in memory...

Is there any reason why the lib/Dispatcher.js can't return the class directly?

Since dispatchr relies on a certain API for stores, we should provide scaffolding or convenience methods for creating stores.

this is more of a question/proposal

For example if we have an autosuggest widget for the contacts that dispatches GET_CONTACTS_INVOKE and GET_CONTACTS_SUCCESS actions as user types the name of the contact.

as user is typing something in the input it'll dispatch multiple actions:
GET_CONTACTS_INVOKE
GET_CONTACTS_INVOKE
GET_CONTACTS_SUCCESS
GET_CONTACTS_SUCCESS

and there is no way of identifying which success corresponds to which invoke action.

Right now we're generating tokens and pass them with the payload

function actionCreator(context, payload) {
  var token = Math.random();
  context.dispatch('GET_CONTACTS_INVOKE', {token: token});
  apiClient.makeRequest().then(function(response) {
    context.dispatch('GET_CONTACTS_SUCCESS', {response: response, token: token});
  }).catch(function(error) {
    context.dispatch('GET_CONTACTS_FAILURE', {error: error, token: token});
  });
}

And on the stores side, we know how to group them together.

This is also important in the case of optimistic updates, because we need to know what we want to commit and what we want to rollback.

As an alternative and cleaner proposal, i was thinking, what if context.dispatch() had a pre-generated token in a closure every time the action creator function is invoked?
kind of like

function DispatcherContext (dispatcher, context) {
  /* ... */
  var token = Math.random();
  this.dispatch = function(actionName, payload) {
    return DispatcherContext.prototype.dispatch.call(this, actionName, payload, token);
  }; 
}

so this action creator invoked twice will produce:

function actionCreator(context, payload) {
  context.dispatch('INVOKE');
  async().then(function(response) {
    context.dispatch('SUCCESS', {response: response});
  });
};

will produce

// => 'INVOKE', null, 123456
// => 'INVOKE', null, 098765
// => 'SUCCESS', {response: {}}, 098765
// => 'SUCCESS', {response: {}}, 123456

What are you thoughts on that?

For dependency mapping, it's better to require a store and pass the constructor to getStore and waitFor instead of store name string. At the very least, the readme should be updated to reflect this and use constructors instead of strings in the examples.

It may also be better to remove string capabilities in getStore and create a separate function getStoreByName for accessing stores by name.

Added in #13

When creating stores and adding to a Fluxible application, the createStore method with a supplied storeName does not seem to register the store in the dehydrated object, whereas using BaseStore does?

We do not queue, we throw.

https://github.com/yahoo/dispatchr/blob/master/lib/DispatcherContext.js#L47

Theoretically, stores could be reset to their original state by recording the actions and payloads that were dispatched on the server, serializing them, and then replaying them on the client to rehydrate the application. An investigation on the performance and implementation benefits for each should be done.

This could reduce the amount of code in each store since they would no longer need to implement dehydrate/rehydrate functions.

Currently Stores look like the following:

module.exports = createStore({
    statics: {
        storeName: 'SomeStore',
        handlers: {
            'SOME_MUTATION': 'some_mutation',
        }
    },

    some_mutation: function(state) {
        this.state.count++;
        this.emitChage();
    },

    initialize: function() {
        ...
    },

    getState: function() {
        return this.state;
    }
});

The problem here is that the store "mutation" functions are still accessible, a way that would help hide direct mutation calls would be to allow the "handlers" hash to accept functions instead of just method name strings

example:

module.exports = createStore({
    statics: {
        storeName: 'SomeStore',
        handlers: {
            'SOME_MUTATION': function(state) {
                this.state.count++;
                this.emitChage();
            }
        }
    },

    initialize: function() {
        ...
    },

    getState: function() {
        return this.state;
    }
});

This limits mutations to not be directly accessible on the store object, but only thought the action handlers, of course the store would be bound to "this" in the handler functions.

This would help separate accessor and mutator functions in stores.

Within a react native app, I do not see a way to apply the component this context to the handler callback function.

// ExampleStore.js - store
'use strict';

var createStore = require('dispatchr/addons/createStore');
var Promise = require('bluebird');

var ExampleStore = createStore({
  storeName: 'ExampleStore',
  handlers: {
    GET_STATE: function () {
      var endpoint = '/endpoint';

      var promise = fetch(endpoint)
        .then(response => response.json());

      return Promise.resolve(promise)
        .then((data) => {
          // this.state is undefined...
          this.state.hasData = true;
          this.state.data = data;
          this.emitChange();
          // ...but can't access component setState(), either.
          this.setState({
            hasData: true,
            data: data
          });
        })
        .done();
    }
  }
});

module.exports = ExampleStore;

// Example.js - component
'use strict';

var React = require('react-native');
var ExampleStore = require('../../stores/ExampleStore');
var dispatcher = require('dispatchr').createDispatcher({
  stores: [ExampleStore]
});
var contextOptions = {};
var dispatcherContext = dispatcher.createContext(contextOptions);
var { View } = React;

var Example = React.createClass({
  getInitialState: function () {
    return {
      hasData: false,
      data: {}
    };
  },

  componentDidMount: function () {
    dispatcherContext.dispatch('GET_STATE', {});
  },

  render: function () {
    return (
      <View></View>
    );
  }
});

module.exports = Example;

It seems it may be desirable to have a way to apply the component this here - https://github.com/yahoo/dispatchr/blob/master/lib/Action.js#L67