Why the Dispatcher don't return the class directly? about dispatchr HOT 11 CLOSED

This is to prevent side effects when requiring the dispatcher. Since it has static methods, if you wanted a second dispatcher in your application or were writing tests and needed a new dispatcher class for each one, then you'd run into issues because they would be sharing the same class with static properties and methods.

We could make it possible to use it both ways: export a class and add a method createClass() that then returns a new class. I'm not sure of the benefits of exporting the class though.

from dispatchr.

I like you suggestion about exporting a class, also the static methods that can be changed should become instance methods instead... for instance .registerStore could be an instance method instead of a class method. It is easier to think about classes when they don't change state, static methods are fine but static properties can get confusing...

Currently each dispatcher object will inherit from a different class with mostly the same code so we are not really making any use of the prototype and wasting memory space with duplicated code when you create multiple dispatchers.

If this file is suppose to work like a factory method it would be cool to rename it (createDispatchr()?) and instead of returning a class it can return a simple object... that is also a waste of memory on duplicated methods...

from dispatchr.

You shouldn't be creating multiple Dispatcher classes unless you need them. In this case, each instance will not be inheriting from a different prototype. They all inherit from one shared Dispatcher class. The idea is that requiring dispatchr from multiple files will not result in registered stores bleeding between each require.

from dispatchr.

@mridgway what do you think about prototypal inheritance instead of pseudoclassical?

Example:

var Dispatcher = {
    registerStore: function,
    isRegistered: function,
    getStore: function,
    ...

    create: function (context) {
        var child = Object.create(this);
        child.storeInstances = {};
        child.currentAction = null;
        child.dispatcherInterface = {
            getContext: function getContext() { return context; },
            getStore: this.getStore.bind(child),
            waitFor: this.waitFor.bind(child)
        };
        return child;
    }
};

this way we can create dispatcher that inherit from the main dispatchr of from an other without leaking:

    var dispatcher = require('Dispatcher').create(context);
    dispatcher.registerStore(...);

    // want a new dispatechr 'instance'?
    var d1 = dispatcher.create(context)
    var d2 = dispatcher.create(context)

   // don't want to leak?
   var dispatcher2 =  require('Dispatcher').create(context);

from dispatchr.

Yeah, I've been thinking about moving to a more functional approach to creating the dispatcher than using new just like your example. I'm definitely open to using an API like you mentioned, we'd just have to plan it for a BC breaking version or introduce it without breaking the current API.

To make it more clear what each method is doing, my thoughts for the API were something like:

var dispatchr = require('dispatchr');
var Dispatcher = dispatchr.createDispatcher();
var dispatcher = Dispatcher.createContext(options);

This would actually allow us to keep the current API and just add in the createDispatcher() method. Then it works exactly as before if you wanted to use new Dispatcher() but we add a convenience method for creating the dispatcher instance with createContext(options).

You can see that it's not much different from what we're doing now, except the initial function call is explicit about what it's doing because it is named createDispatcher().

from dispatchr.

does Dispatcher.createContext(options); returns a dispatchr or context object?

from dispatchr.

createContext would return the same thing that new Dispatcher() returns right now, which is the per-request dispatcher.

from dispatchr.

The proposal of this PR is to do:

var Dispatchr = require('dispatchr');

// Creates a new dispatcher instance
var Dispatcher = Dispatchr.create();

// Register some stores
Dispatcher.registerStore(MyStore);

// Creates a new dispatcher Instance for a context
var contextDispatcher = Dispatcher.create(context);

from dispatchr.

Ok, so in the above PR I mentioned a similar approach that keeps that doesn't change the internal pattern too much: https://github.com/yahoo/dispatchr/compare/factoryClass

I think we need to think about the API for this before making any changes. I think using generic create methods might be a little confusing to know what is being returned.

Currently in master, it looks like this:

var dispatchr = require('dispatchr');
var Dispatcher = dispatchr();
var dispatcher = new Dispatcher(context);

Right now the branch looks like:

var dispatchr = require('dispatchr');
var dispatcher = createDispatcher();
var dispatcherContext = dispatcher.createContext(context);

This might get confusing because context is used everywhere in our fluxible stuff.

I like this is better:

var dispatchr = require('dispatchr');
var dispatcherFactory = dispatchr.createFactory();
var dispatcher = dispatcherFactroy.createDispatcher(context);

I know @ericf mentioned not liking the API we have currently so maybe he has some input.

from dispatchr.

I like something more similar to what you proposed first:

// Using constructors 
var dispatcher = require('dispatchr').createDispatcher();
var dispatcherContext = dispatcher.createContext(context);

from dispatchr.

I think the API would be easier to use in fluxible if we updated the index.js to something like:

module.exports = {
  createDispatcher: require('./lib/createDispatcher'),
  createStore: require('./utils/createStore'),
  Action: require('./lib/Action')
};

from dispatchr.