Consider an async (promise) allowance for actions / stores #122
Comments
|
This breaks away from a few of the core principles of flux. Mainly, the actions then are not fire and forget and this would also mess with the central dispatcher. I don't see how this can be implemented cleanly without stripping out the dispatcher. You can already use ES7 async/await syntax with actions btw. I am interested in hearing more about your use case though since we may be onto something here. |
I don't often have an action that I can forget about from a UX or even functional standpoint. If I understand correctly, Stores should be the only cog in the system that can or should interact with the data layer; meaning, the only place I would perform an AJAX request would be in a store. If I send a If I send a
When I said I'd jump in and implement it, I meant I have it working locally with monkey patching and can clean it up and submit a PR.
I realize this (our codebase has this everywhere and its awesome) but I usually go for the principle of least astonishment when explaining a request. The average JS coder on github would have seen
It wouldn't feel right (by my understanding of flux) for actions to be interacting with the REST API with AJAX requests (and that is the only reason I have to be async). That is the stores job (again my understanding could be wrong). |
|
Actions can totally handle async operations, and it actually makes more sense to have them there instead of the stores. Upon finishing an async operation, another action will need to be fired to pass the result to the dispatcher. Doing this in the action creator helps keep a clean separation of concerns. |
|
I've been thinking of a way to achieve a similar end result, though I have to agree with @troutowicz that action creators seem the most natural place for something like this. The method I had in mind (and actually briefly discussed with @goatslacker on Gitter) was as follows:
This would have several nice properties IMO, such as:
doAction() {
return http.get('/data')
.then(this.actions.handleResult)
.then(this.actions.subscribeToUpdates); // if this action also returns a Promise, we'll automatically wait for it as well
}The most obvious use case for this would be isomorphic rendering, where it's not trivial to know when the application is ready to be sent down to the client (we've referred to this as the "Isomorphic Halting Problem" elsewhere :). This would allow you to do something along the lines of: var app = new MyAltApp();
var ops = [];
var latest;
app.dispatcher.register(payload => {
latest = payload;
ops.push(payload.result);
Promise.all(ops).then(() => {
if (latest === payload) {
// ready to send response!
}
});
});
app.actions.routing.route('/foo/bar');Other isomorphic-capable Flux implementations (such as Fluxible) necessitate the "routing" action being the one that determines when things are ready. I feel this method would make it possible to wait on a more general level, regardless of where, how and how many actions are dispatched on the server-side. What do you think? |
Yes. Actions can handle asynchronous operations. I was under the impression that views do not listen to actions (as the flow should be uni-directional)? This means I'll have lots of "view state" in stores (meaning they are not very re-usable); the stores become very dumb (with all of the REST API interaction logic in the actions); and my actions become quite complex (going against what I've read elsewhere -- your actions file should be one, long file). @jareware I agree. I'd be fine with exactly as you describe with the addition that
I'm going to say that because alt pushes ES6 then it should push / require Promises (as they are a very ES6 concept). Using anything else for async operations at this point (with @goatslacker The changes I'm asking for (in addition to what @jareware presents) would remove the necessity for |
|
I'd be interested in seeing some code :) |
|
@goatslacker I'll get working on a PR then. |
|
I am interested in seeing some code as well, but wanted to weigh in my initial thoughts as well. I agree with the initial sentiments of @troutowicz and @goatslacker. I think that the introduction of promises, especially in stores, goes against basic Flux principles, like actions being fire and forget. Stores should just reflect data at a certain point in time. As actions occur, the data may change, which is then signaled to the view via
The typical pattern I have seen is to fire an action that updates the store to signify that the request has begun. At this point eager loading can be used as you mentioned, or a loader or something in the UI to represent that the request is loading. Upon completion of the request, we can look at the data and fire a fail action, or success action, which will again update our store data and ultimately our view to get it into its final state (until another action occurs). So essentially for async requests, we fire an action when we begin the request, and fire another (fail or success) when the request returns. This sticks with the pattern of firing actions and forgetting. I remember that when I first started working for Flux, I initially wanted to wrap my actions in promises for some of the reasons you mentioned, but as I have worked with it more, I have found that I have not had need to do this (at least not yet). In async calls to other web services my app needs to communicate with, I simply call another action on the requests completion: class MyActions {
...
myAction(data) {
// this dispatch will set data in the store that lets my views know
// to display a loader/ optimistically render
this.dispatch(data);
request.get(url)
.query({name: category})
.end((res) => {
// fire fail or success action once request returns
// this will handle getting our store/ui into the correct state
if(res.ok) {
this.actions.fireSuccess(res);
}
else {
this.actions.fireFail(res);
}
});
}
}Ultimately, I do not see the need for Promises in Stores, but can see where they might seem useful in Actions. I still do not believe that Actions need to return promises as we can accomplish the same thing without them as shown above, but I definitely would like to see your PR because maybe a concrete example will help me better understand where you are coming from. Though it is tempting, I think introducing promises in this manner goes against the Flux pattern, while alt maintains to stay true to vanilla Flux. I do want to make it clear that I am not completely against promises, I think they are a useful tool, but I do not see the need in the situation described. I also wanted to cite some things I have read in the past from various individuals at Facebook that have shared why they have gone against the Promise approach at Facebook.
|
|
Thoroughly agreed (esp. on why dispatch should be synchronous). Except:
How would you know when the application's done retrieving its initial data when rendering server-side, though? |
This is one case where being able to return promises from actions makes sense (if you keep your stores synchronous) you can aggregate the all the actions and then render when they all complete. |
|
Great point @jareware. I can get behind that! I just want to make sure we stick close to Flux, but it sounds like we are moving in the right direction. 👍 |
|
Any pseudo code you can share here. I'd love to take a look and start playing with it. |
|
I strongly believe that returning promises from actions is wrong and breaks Flux principles.
I had the same thoughts, but then I realized that this limitation keeps my code cleaner and simpler.
@jareware there must be a better way to solve it :) |
|
@ziflex, with regard to:
Returning Promises from actions in no way precludes this, and this is in fact exactly what I do with The init/done/fail action triplet is still the one that changes the internal world state (as per Flux conventions), and the Promise is only there to let an external party know an action they dispatched (and any of its consequences such as a done/fail action) have been processed. It's just a more abstract (and therefore more general) way of answering that question than adding specific listeners for a bunch of actions you think will happen. Does that make sense? |
|
I've submitted #159 (pending review before I add tests) to ease promises with stores. I'm struggling to think of the best way to have actions interact with stores. Currently you can just return a promise from an action and have it work. It's not being tracked anywhere but it allows then-able behavior. Problems that I'm trying to resolve:
The action creator (method on the action object) itself invokes dispatch. This is what then invokes any registered listeners with the action. Trying to wait for everything may be better accomplished by running your actions and then waiting for the stores to finish updating. Just talking on paper here. If we can make |
How do you know that from stores updating? Isn't it fairly common for doAction() {
return http.get('/data')
.then(this.actions.receiveData)
.then(response => {
if (response.needsMore) {
return http.get('/more-data')
.then(this.actions.receiveData)
}
})
}Because the Promises are chained (notice we always return) you can be sure that whatever
Indeed! The only change this would need is the ability to "collect" all Promises returned by action dispatches somehow (see above for my suggestion on this). In this case you wouldn't actually even need to be careful to always return the Promise, as the dispatcher would be aware of new operations being kicked off.
Some Flux implementations choose to have automagic for this repetition, but given how simple it is to |
If you have some logic which might be called outside of your app or outside of your FLUX architecture, why not just move it outside of UI layer and make it more 'low-level' service which is not part of FLUX architecture ? But I agree that how to sync actions is not completely clear and 'waitFor' isn't best solution. Maybe it's better to ask FLUX creators what the best practices are ? :) |
|
That is of course a wholly valid option as well: to gather the Promises at whatever it is that generates them. The only reason why I'd imagine we're even discussing adding that to |
|
We are using IndexedDb, so everything is async in our Store layer. Is there a clear solution ideally one that does not involve another library? They way I understand it, it is important for the dispatcher to know when the store has completed its task as that is a sync point. I'm not sure how to do this outside of the framework. Since we are adding lots of code to our project in this area so it would be nice to get it correct ASAP. Thank you for your help! |
|
Why can't you persist when the dispatch is done and then handle success/failure the same way you'd handle success/fail if you were making an async call to fetch data.? |
|
Let me try and repeat your idea, I am probably unclear on the process... So, my action object will call this.dispatch and alt will dispatch this to my store. At this point, the dispatcher is "done" as you mentioned above? So, the dispatcher moves on even though the event was handed off to indexeddb. Finally, when IndexedDb calls onsuccess then what? How is the view notified? How does the dispatcher know the store is done and ready for more? I may mis-understanding, but I thought the dispatcher was a sync point/ traffic controller: https://youtu.be/nYkdrAPrdcw?t=19m17s Wouldn't the dispatcher need to know when indexedDb is done? Thank you for your help! This really has to 'sink in' .. |
|
A naive example const DB = {
save(text) {
IndexedDB.saveSomewhere(text, function (err) {
if (err) FooActions.shitAnError(err)
else FooActions.done()
})
}
}
class FooActions {
constructor() {
this.generateActions('done', 'shitAnError')
}
updateTodo(text) {
DB.save(text)
this.dispatch(text)
}
}
class FooStore {
constructor() {
this.bindActions(FooActions)
this.areWeDoneYet = true
this.todoText = ''
}
updateTodo(text) {
this.areWeDoneYet = false
this.todoText = text
// does not emit a change
this.preventDefault()
}
// yay we're done saving...
done() {
this.areWeDoneYet = true
}
shitAnError(err) {
// handle this maybe?
}
}You can just listen to this store like you would listen to any store normally. |
|
@goatslacker, your "naive example" really helps give me deeper insight into this. I think I see an issue: DB depends on the actions calling it. That is a circular reference which will not even load. I think it is a requirement to notify all actions that have a dependency even if it were not the action to dispatch the call (please clarify).
|
|
As mentioned above, why not just treat the db entirely as if it were a network remote? There's lots of patterns around dealing with async stuff happening there, and the concepts map quite closely. In other words, deal with IndexedDB exactly as you would deal with a web API, which you interact with in a similar async manner. Since it's just async calls either way, everything should be almost exactly analogous. |
|
The DB doesn't have to depend on an action to be called...I just assumed you would want it that way since the flow would be: action call -> update store with temporary data -> persist to db -> handle persist success or failure (ie: commit state or rollback state and inform user there was an error saving) You could just call the DB directly. The above code does have a circular dependency but you can easily refactor that out :) const DB = {
save(text) {
// use promises too, whatever!
return new Promise((resolve, reject) => {
IndexedDB.saveSomewhere(text, function (err) {
if (err) reject(err)
else resolve()
})
})
}
}
class FooActions {
constructor() {
this.generateActions('done', 'shitAnError')
}
updateTodo(text) {
DB.save(text).then(() => this.actions.done(), () => this.actions.shitAnError())
this.dispatch(text)
}
} |
|
@mehcode I also want something that you are talking about, for cases such as when I use a modal to create some new data, the modal should close upon success, or remain open upon error to save user from retyping. However, you do not need Promise in action to do this. You can use traditional callback. In your action In your view Hope this helps. |
|
a few of these examples are handled with the current data source implementation, no? is that still a recommended way to do async with actions? |
|
We've run into similar dilemmas trying to determine how to handle async errors. Would anyone consider modifying some of the examples to show the ideal way to handle errors? The weathertabs app, for example, might be a good place to demonstrate how the authors of the library recommend handling errors and displaying and error message on a component: error: () => {
this.actions.setLoading(false);
// send error to store? promise as originally suggested in this issue?
}It would also be interesting to see how to flush an error message. For example, let's say I'm using react-router and the a new route is loaded, at which point the error message no longer applies to the data on screen. I realize this may sound like trivial request, but very few (if any) flux frameworks or sample programs demonstrate error handling and display. It seems to be a source of confusion for developers that are new to flux (myself and my teammates included) and some examples would go a long way :) |
As ECMAScript 6 (and even 7) features become easier to achieve and use in a project, I feel that we should have allowances for using (what is now) first-class asynchronous primitives: Promises.
Stores may return promises in a handling method.
Actions return a promise by-default from the dispatch call.
Internally,
this.dispatchusesPromise.allto wait for all stores to finish execution and returns a chained promise that disposes of the resolved iterable.The above allows for views that invoke actions to easily wait for a success or failure of a possibly asynchronous event.
Thoughts? I'll jump in and implement if we can agree that this is a good proposal for moving forward.
The text was updated successfully, but these errors were encountered: