Skip to content
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

Asynchronous operations #29

Merged
merged 24 commits into from
Dec 1, 2014
Merged

Asynchronous operations #29

merged 24 commits into from
Dec 1, 2014

Conversation

jhollingworth
Copy link
Contributor

Stores

Adding a new function to the store called query which is responsible for

  • Ensuring only one query for a given key occurs at any one time
  • Executing a query against the local cache
  • Executing a function to retrieve data if its not in the local cache
  • Returning a object, StoreQuery that represents the lifecycle of the query
Store#query(key, localQuery, remoteQuery) -> StoreQuery

StoreQuery has a status which can be pending, error or done. Like stores, there is an addChangeListener which allows you to register a callback when the status of the query changes.

If the StoreQuery fails, you can see why by accessing query.error. If the query is successful, the result is accessible by accessing query.result.

Only one query per key can be pending at any one time. While a query for a given key is in progress, invoking Store#query with that key will return the in progress query. This is to avoid making multiple queries unnecessarily. The key is unlocked once its associated query has completed successfully or failed.

If the local query returns something other than null or undefined then the StoreQuery moves to the successful state and the result is available in StoreQuery#result. If an error is thrown within the local query, the StoreQuery moves to the failed state and the thrown error is available in StoreQuery#error

If the local query returns null or undefined then the remote query is executed. Once the remote query completes, the local query is re-executed. If the local query now returns something other that null or undefined then the StoreQuery moves to the successful state and the result is available in StoreQuery#result. If the local query returns null or undefined then the StoreQuery moves to the failed state and StoreQuery#error will return the error "Not found". If an error is thrown within the remote query, the StoreQuery moves to the failed state and the thrown error is available in StoreQuery#error.

If the remote query returns a promise then the completion of the remote query will be delayed until the promise has resolved.

var FooStore = Marty.createStore({
  getFoo: function (id) {
    return this.query(id, getFromCache, getFromServer);

    function getFromCache() {
      return this.state.get(id);
    }

    function getFromServer() {
      return FooAPI.getFoo(id);
    }
  }
});

If you are using a state mixin and the state contains a StoreQuery, the mixin will automatically add a change listener and update the components when the query status changes.

Action creators

Actions are responsible for coordinating state mutations. Actions can only mutate local state but have the potential to mutate remote state. It is also common to do an optimistic local mutation and then deal with aliures later on.

var UserActionCreators = Marty.createActionCreators({
  saveUser: function (user) {
     var addUser = this.dispatch(user);
     return UserAPI.saveUser(user).error(addUser.rollback);
  }
})

An actions type is defined by ActionCreator#getActionType(functionName). The default strategy is to upper case and separate using underscores, so saveUser becomes SAVE_USER. You can override getActionType or choose a specific action type by specifying it as the first element of an array

var UserActionCreators = Marty.createActionCreators({
  saveUser: ['FOO_BAR', function (user) {
     var addUser = this.dispatch(user);
     return UserAPI.saveUser(user).error(addUser.rollback);
  }
})

Every action creator function will return an action token. You can pass this token to Marty.getAction(actionToken) to get the status of an action. The state mixin automatically listens to the action store (unless you set listenToActions: false) but will only update the views state if you return an action from getState which is the action that just changed

Marty.createStateMixin({
  getState: function () {
    return {
      someAction: Marty.getAction(this.state.someActionToken)
    }
  }
})

You can listen for changes to actions by registering handlers for ACTION_STARTING, ACTION_ERROR and ACTION_DONE.

If you return a promise from an action creator, the action will wait for the promise to complete before updating the action status.

To do

  • Create StoreQuery
  • Add locking to Store#query
  • Add local queries
  • State mixin should listen to store queries as they change
  • Add remote queries (including returning actions)
  • Listen to actions
  • Documentation

@jhollingworth jhollingworth changed the title Store#queryLock Asynchronous operations Nov 23, 2014
@rymohr
Copy link

rymohr commented Nov 24, 2014

This looks like it would be a great addition. How about simplifying StoreQuery#status to pending|done|error?

@jhollingworth
Copy link
Contributor Author

@rymohr 👍 thats much nicer

@jhollingworth
Copy link
Contributor Author

@rymohr I've been pondering how to handle async actions, would love your thoughts

Say you have a component that creates an action and you're interested in its progress (e.g. you create something on the server and you want to know when its done or error'ed). How do you handle this in a flux-ey way?

var User = React.createClass({
  saveUser: function () {
    UserActionCreators.createUser({
      name: "Foo"
    });
  }
});

One option would be to allow you to register callbacks for when an action status changes or make it into a promise

UserActionCreators.createUser({ ... })
    .then(onUserCreated)
    .catch(onFailedToCreateUser);

This kind of bothers me for two reasons

  1. Data is flowing backwards i.e. rather than going view -> action -> dispatcher -> stores -> view its going view -> action -> view
  2. I feel that all views should not be responsible for handling async operations

An alternative is each action creator returns an "action token". You would then have an Action store which allows you to get information about actions (e.g. their status)

this.setState({
    createUserToken: UserActionCreators.createUser({ ... })
});

var createUser = ActionStore.getAction(this.state.createUserToken);

if (createUser.failed) {
   ...  
} else if (createUser.done) {
   ...
}

It feels more in line with the flux architecture but unsure whether its a nice API for users. What do you think?

@rymohr
Copy link

rymohr commented Nov 25, 2014

That's a tricky one. After reading dozens of articles and interviews I'm not sure the facebook team has even found a great solution yet.

I think the goal should be to encapsulate async operations into state as much as possible. If state includes data about pending requests, then there's no reason to add another way to monitor progress. The state will change as progress changes.

I'd lean towards the second approach over the first one for sure. Views should not deal with async flows. I think I'd add a separate Marty.getAction(token) helper instead of exposing a separate ActionStore.

Whether that approach would scale to a production-size app is another question. At some point you'll have to clean up old actions to prevent the history from growing indefinitely and that adds yet another edge case to consider in views.

@jhollingworth
Copy link
Contributor Author

Awesome, glad it makes sense 😄

What I'm now thinking is create an action store thats accessible at Marty.Stores.Actions. Just before an action is created, an action will be dispatched with the action type "{actionType}_STARTING" (e.g. "ADD_USER_STARTING"). When an action completes it will dispatch "{actionType}_DONE" and "{actionType_FAILED}" if it fails.

ActionStore#getAction(token) will return an object literal which returns some basic information about the actions status

{
  "failed": true,
  "type": "ADD_USER",
  "pending": false,
  "error": ...
}

I completely agree that you shouldn't have to interact directly with ActionStore. I really like Marty.getAction(token) so would have that as a shortcut to Marty.Stores.Actions.getAction(token). I'd also have the state mixin automatically listen to the ActionStore so you can easily update the view

var UserState = Marty.createStateMixin({
  getState: function () {
    return {
      saveUser: Marty.getAction(this.state.saveUserToken)
    };
  }
});

var User = React.createClass({
  mixins: [UserState],
  render: function () {
    if (saveUser.pending) {
      // show loading gif
    } else if (saveUser.error) {
      // show error
    } else if (saveUser.done) {
      // show something else
    }
  },
  saveUser: function () {
    this.setState({
      saveUserToken: UserActionCreators.createUser({
        name: "Foo"
      })
    });
  }
});

Removing actions from the store is a really good point and I'm not sure what the answer is. Perhaps its worth implementing a naive time based strategy (e.g. remove anything thats not been used in > 5 minutes) but provide the opportunity to implement your own strategy

@rymohr
Copy link

rymohr commented Nov 25, 2014

Looks good! Here's some alternate naming strategies to consider:

Before / after

BEFORE_ADD_USER
ADD_USER_SUCCESS
ADD_USER_FAIL
AFTER_ADD_USER

Optimistic

This is the default behavior of react-flux

ADD_USER
ADD_USER_SUCCESS
ADD_USER_FAIL
ADD_USER_AFTER

Promise-like

ADD_USER
ADD_USER_FULFILLED
ADD_USER_REJECTED
ADD_USER_ALWAYS

Short

ADD_USER
ADD_USER_DONE
ADD_USER_FAIL
ADD_USER_OVER

I don't have a strong preference, but I slightly lean towards the short form because:

  1. it's short and easy to type
  2. it encourages optimistic action handling by default
  3. it naturally flows with on{ACTION} style handlers (onAddUserDone vs onBeforeAddUser)
  4. it reuses the done/fail behavior established by jquery

Whichever you choose to go with, I really like how react-flux provides a helper for defining constants to encourage a consistent naming scheme: https://github.com/kjda/ReactFlux#constants

@jhollingworth
Copy link
Contributor Author

I hadn't seen https://github.com/kjda/ReactFlux#constants before, really like that. agree on the short form, would be nice to keep a consistent terminology throughout

jhollingworth added a commit that referenced this pull request Dec 1, 2014
@jhollingworth jhollingworth merged commit e2e3160 into master Dec 1, 2014
@jhollingworth jhollingworth deleted the lock branch December 1, 2014 22:35
@KidkArolis
Copy link

Looks really good!
How would you handle querying multiple types of resources within the same store? Would you prefix the I'd, e.g. query('a1'.., query('b1'...?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants