kjda / ReactFlux

A library implementing React-Flux data flow design pattern + Code Generation
MIT License
66 stars 12 forks source link

Add global action progress tracking #7

Open lelandrichardson opened 9 years ago

lelandrichardson commented 9 years ago

I have found that an incredibly common situation in most client-side applications is displaying some indicator in your UI that a certain AJAX request is taking place or in progress. The actionHandlers built into the stores of this library are a great way to handle this, but if all you are looking for is a simple boolean as to whether or not the action is in progress or not, then this can result in some quite common "boiler plate" code that I thought could be avoided.

As a result, I created an "ActionProgressStore" in my application, which worked similar to how the code in this PR works.

If you feel like this is outside of the scope of this library, I am totally okay with that, but I thought I would do a pull request just in case you thought it was a good idea.

The general idea is there is a property off of the main global ReactFlux object which is essentially just a Store with a couple of special methods on it.

It's intended to be used in the following scenario:

Say we have a component named FooList, which retreives Foos from the server. Not only does the FooList display the returned Foos from the server, it also displays whether or not it is currently retreiving any Foo's:

var FooList = React.createClass({

    mixins: [
        FooStore.mixin(),
        Flux.progress.mixinFor([constants.SEARCH, constants.NEXT_PAGE])
    ],

    getStateFromStores: function () {
        return {
            foos: FooStore.getList(),
            isBusy: Flux.progress.any([constants.SEARCH, constants.NEXT_PAGE])
        }
    },

    render: function () {
        return (
            <div>
                {this.state.isBusy && "Loading..."}
                <ul>
                {this.state.foos.map(f => <Foo foo={f} />)}
                </ul>
            </div>
        )
    }
});

The pull request is basically implementing a default global "Store" which potentially tracks the progress state of every action created through ReactFlux.

In actuality, the store only tracks the progress of the actions that it is asked to track. Moreover, it does not use the default store change event, but rather a change event specific to the action it is "tracking". This is mainly to prevent a situation where (in a large application) a single "change" event would cause the re-rendering of a very large portion of the app unnecessarily.

This is certainly a feature that I don't think is "required" to be present in a flux library, but I do think it is a common enough use case such that most people would find use of it and it could help adoption of this library.

I am very open to criticism or discussion over such a "store" being exposed by default. Please let me know what you think.

lelandrichardson commented 9 years ago

I was thinking about this more last night. An alternative syntax could be to append an isInProgress() method on the constants.

This could then look like this:

var FooList = React.createClass({

    mixins: [
        FooStore.mixin(),
        constants.SEARCH.mixin(), 
        constants.NEXT_PAGE.mixin()
    ],

    getStateFromStores: function () {
        return {
            foos: FooStore.getList(),
            isBusy: constants.SEARCH.isInProgress() || constants.NEXT_PAGE.isInProgress()
        }
    },

    render: function () {
        return (
            <div>
                {this.state.isBusy && "Loading..."}
                <ul>
                {this.state.foos.map(f => <Foo foo={f} />)}
                </ul>
            </div>
        )
    }
});

Thoughts?

kjda commented 9 years ago

Hi Leland, I was thinking about the same issue lately :) I will look at this later and get back to you asap

lelandrichardson commented 9 years ago

Ultimately, I'm not sure this is a great idea to be part of the library - but I'm torn.

On the one side, it's a very common need... and by adding it as a built in feature, it allows users of this library to quickly have this type of thing figured out without having to make their own store to handle it.

On the other hand, it may be "outside of the scope" of what this library should be.

Oh, and I decided that my "add it onto the Constants" idea was not very good and could lead to some confusion maybe. Let's drop that idea...

mrtnbroder commented 9 years ago

+1 for not adding this into the library, but as new package this would be awesome!

kjda commented 9 years ago

@lelandrichardson What do you think about adding this as an addon, like react/addons?