-
Notifications
You must be signed in to change notification settings - Fork 421
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
Make promises observable #179
Comments
I'm not sure about that syntax. How does it handle rejection? How does it handle being queried about its state? What happens when a function returns a deferred (as is the case with findAll)? EX 1var deferred = Todo.findAll()
can.stache(TEMPLATE)({deferred: deferred})
EX 2Could we support an
Making However, making the |
@andykant I assume there's nothing in handlebars / mustache about allowing arbitrary sub-sections? |
If we implement it as a helper, we can support something like this:
|
Nothing currently for arbitrary sub-sections, right now all you can do is "else", but we could add a special case for Deferreds.
As far as arbitrary sub-sections, maybe we could implement a mechanism that allows you to inject a custom scope for a given object type (in this case deferreds). Initially it could just be a hardcoded version for Deferreds.
|
How is this different from the can.List promise plug-in? Is it the fact that you could do |
@sporto can.List only works on lists, not single items or anything other than a can.List. This is a more general solution. The deferred can be passed to the view like:
However, I don't think in a practical sense, even
Owners could be an attribute of Todo implemented like: can.Map.extend({
define: {
owner: {
get: function(current){
return current || Owner.findOne({id: this.attr("ownerId")})
}
}
}
}) The nice thing about this is that you can just read "through" the deferred in the view. cc @bmomberger-reciprocity b/c I've seen this kinda thing done quite a bit in his code base. |
@justinbmeyer I think that making the API in the template consistent is very important. With the can.list promises plug-in you have already introduced syntax like:
This proposal seems inconsistent with that, which is not great. I often want components can either fetch their own model/models or receive a model/collection already in fetched. So, it would be really great to not even have to bother if something is a model/list or a promise, but still have the power of saying .isPending to show a loader. |
@sporto What is inconsistent? Deferreds would still have a .isPending and .isResolved method like:
The
There would probably be a It's also possible we can make the following work:
I'm not sure what you are suggesting / saying. |
@justinbmeyer The other consistency issue is |
Is it even necessary to support this kind of functionality? In almost all cases you could utilize asynchronous getters/setters to create belated change events, at which point this is a simple matter of using section helpers. It's perfectly valid as well to both return a placeholder value (false maybe?), and later call setValue on done to force the section helper to inverse while pending. |
It isn't and it also shouldn't be created. As I commented in #1207, if you really want observable result values from a Deferred object, you should wrap a custom |
Is anything really "necessary"? We add features to make things easier. You can not easily accomplish a deferred's entire pending/resolved/rejected/value/reason state with the define plugin. I've tried it, it's very difficult. If you want an app with good error handling and have Ajax requests do not fit into lists, you'll know the pain of this situation. Sent from my iPhone
|
A promise/deferred is already an observable control flow structure. We aren't attaching observability, we are simply making it visible to the view and to computes. To make deferreds visible, all they need to do is call can.__reading. So there will be no overhead. We are going to probably make everything observable with dirty checking eventually. Having as many types of things observable to the view as possible is a good thing. Please share the compute that wraps the pending/resolved/value/rejected/reason state of a compute. I think after writing it, you will see why simply allowing promises would be way nicer. The reason is that you essentially have to recreate the promise structure. Sent from my iPhone
|
I think this feature is a bad idea as well. This feature promotes using model / controller type functionalities (data retrieval) into your view. It just promotes bad architecture.
I agree with you there. I have a fairly complex control flow in one of my viewmodels with 6 deferreds with some interdependencies, and the code is pretty ugly and complex right now. Still looking for ways to reduce complexity. For the time being I've chosen to delay rendering until the deferreds are all resolved. |
Hadn't even considered this part yet, but yes; that's true. It does tempt people to push the definition of responsibilities that should lie with controller or model into the view. From an application guidance standpoint, that's quite bad. An explicit wrapping around a Deferred that you want to have observable atleast has better odds of being set-up in the correct location: as part of the definition of the (view)model. Let's please not have |
I actually agree with @gsmeets on this one. In GGRC, we mostly abstract the use of deferred away from the view layer. Even though we support a {{#defer}} helper that takes a deferred and renders when it resolves, it's rarely used. We prefer the more abstract {{#using}}, which syncs a stubbed object/list property of another object from the server, or {{#with_mapping}}, which refreshes a mapping (defined path through the object graph which yields a list) attached to an object. Really it seems to me that this is the problem that Bacon.js is at the ready to resolve. All your deferred objects are really iterators on streams waiting for data. Thus any deferred operation is really just a list iterator with a defined terminus. The view layer should treat the deferred as an iterator. {{HasNext}} (or {{pending}}) is true until the deferred resolves, false after resolution. Errors on the deferred pass an error as the final iteration (there are likely no success case iterations that happen, in this scenario), which can be caught with a special section, e.g. {{onError}}. Did this make sense to anyone else? |
Having FRP (bacon) support in the view creates the same problems people are worried about as having promises. (But that's not to say we shouldn't support FRP, instead we should) To me, a promise is state, plain and simple. If state changes, the view should be able to update itself. Waiting for promises to resolve before rendering is exactly the type of problem I want to solve. I think the concerns about breaking layer boundaries are valid. It can happen, but there are many cases where a deferred is part of the view-model and we shouldn't shy away from making this very useful feature because someone might blow their hand off. Consider angular where the view can define and add to the view model. Sent from my iPhone
|
There are reasons why I've chosen for canjs instead of angular to base our company's new main product frontend architecture on. ;) I'm not opposing to having some way to be able to automatically update promises. As a matter of fact, I'm already doing something like that, but it's a bit annoying for single model instances. Lists are working nicely ofcourse, because add events are easy to handle out of the box. What I'm doing right now is basically this: var foo = new Foo();
Foo.findOne({ id: 42 })
.done(function( model ) {
foo.attr( model.serialize());
});
can.view( VIEW, data, function ( fragment ) {
this.element.append( fragment );
}); An elegant way to handle this would be to add a counterpart to findOne that updates a given model with data from the server, i.e.: var foo = new Foo({id: 42});
foo.synch(); // synch would perform a findOne, and update foo with the data, instead of returning a new Foo instance. |
@gsmeets Thanks for your thoughts.
t1a = new Task({id: 1})
t1b = new Task({id: 1})
t1a !== t1b This is different from how can.Model.findOne currently works: $.when(Task.findOne({id:1}), Task.findOne({id: 1}) ).then(function(t1a, t1b){
t1a === t1b
}) This is why promise support is the most elegant and simple (less APIs to add) way of handling this situation. |
I briefly mentioned a concept on #1080 in which we'd make getters and setters both able to use async getters and setters and pipe values. I think however we can accomplish all the functionality desired here by expanding on that concept, and by treating this in a similar fashion as validation by creating a promises object.
In this scenario, if the getter returns a deferred, we'd wait until it resolves before updating the getterValue, which is what will be returned when calling attr. This gives us very desirable getters, and change events. We then would expose a promises attribute that has a hash map of all of the deferreds. We could access it in the template as {{#foo}} or as {{#promises.foo}}/{{#promises.foo.isPending}}/{{#promises.foo.isResolved}} etc. Thoughts? |
Optimally I would like to have a template like:
And not really care if what I give is a promise or a model. So my component could either do;
or
In the first case the template would expect a isPending, isRejected, etc function in can model, maybe they could be there just so the view can easily accommodate this use case. In the second case it would be something like the existing list plug-in. |
One of the confusing things about the define plugin is that this is exactly what happens if you were to add a Type and a setter function. Ideally what I want is to be able to pass an ID to my viewmodel, which in turn expands that into a full model by fetching it from the server. I'm currently refactoring a bit to make this possible. But the weird thing is I need to drop my explicit type definitions to get this to work. Otherwise the Type converter will have already converted my single id into a can.Map before I'm able to touch it in the setter function. |
I'm not sure what you mean. If there's an issue/limitation to the define plugin, please describe it in another issue. Sent from my iPhone
|
I think I can work around the issues I'm having with it. It's just that some things work in a bit of an unintuitive way. But maybe that's just my strongly typed past shining through. ;) |
Ah, but that doesn't cover error states, now does it? Let's go with the above idea from @Bajix that the define plugin's getter is given some special handling for promises:
Using real asynchronous getting/setting and having an additional hashmap of all 'map attributes that are promises', is rather cumbersome. Rather than having the model attribute return {
state : ... // Returns one of "pending", "resolved" or "rejected".
value : ... // Returns a resolved or rejected promise's first argument value.
values : ... // For multiple argument values on a promise. (Always returns an array.)
} This allows you to easily switch visibility of loader elements or classes using the You could also incorporate support for things such as the progress callback handling on I see that
|
@rjgotten I like adding switch. I was thinking that value, state, and reason would be special keys like
|
Well, so part of the goal of what I was proposing is to make it so that accessing a property using, for example, findOne, would always return either null, or the currently set instance. The point of this approach is so that after resolving, your values are always synchronously available, and aren't updated unto the next async setter resolves. There are a lot of huge benefits to this sort of approach. For starters, your accessors are very lean, and can be readily used in other computes. You can also interact with the current value, and with the underlying promise independently, making it so that your views can opt to show the current value until the new value is loaded. By having access to the underly promises, you could construct composite when promises, potentially even by using computes observing the promises object. It really just lets us use whichever approach is applicable for the situation. |
@justinbmeyer Rather than using switches, I usually add in helpers for ternary operators, and for matching values.
Thoughts? |
For @rjgotten: https://github.com/reciprocity/ggrc-core/blob/1e2ce1150f84775ada6037181dab05cd9c0097c9/src/ggrc/assets/javascripts/mustache_helper.js#L2173 Also, just to 👍 everyone, these are all very good thoughts. I feel like we may be trying to solve too large a vertical slice, though. It should be broken down a bit, e.g.:
|
@bmomberger-reciprocity Thanks for clueing me in on the switch helper being brewed there. @justinbmeyer Those special keys make it look kind of nice, I admit. But that ties the interpretation of deferreds as computes back into the view engine, which takes away the possibility of establishing entire chains of computes that have 'deferred computes' as part of them. @gsmeets Regarding the kludge you're currently using for the async fetched model; have a look at a more complete version of the idea I presented in #1207. Maybe this is just what you're looking for to smooth things over in the mean time. (Do note: I still wrote this off the top of my head. So I hope there aren't any bugs left in there.) |
@rjgotten On:
I'm not sure what you mean. That syntax does not mean that a compute could not update itself when a promise changes. A promise's |
@justinbmeyer On:
I still think you won't want that. If you have all promises call into This is part of what I meant when I mentioned before:
The flipside of too many calls to Biggest immediate issue with that is that you'll no longer be able to directly access and forward the But that's just jQuery... That does not yet cover promise-like results from every other library under the sun that returns its own flavor of 'thenables'. I maintain that a user-controlled wrapping point where observability is added is the best solution that strikes a balance between usability and performance. |
In my opinion, there's no need to strike a balance with performance. can.__reading is extremely light. It will not slow any conceivable app down. https://github.com/bitovi/canjs/blob/master/compute/compute.js#L76 stack.length would be 0 unless you were in a compute / view. We'd be adding one function call and branch per promise (that wouldn't be in a compute). You'd have to have 1000s of same-exectuation-thread
Yup, those other libraries could convert those promises to the compatibility library's promise for observability. |
Edit: to make jQuery work, there will probably also have to be an extra function call and a property assignment when any deferred is created. This still will very likely not be enough to impact the performance of any application. |
It will be good to have a way to know when a template is "done", after all promises have been resolved. |
You can always wire that up yourself with
|
I'm thinking of the server rendering use-case, would want something more generic. |
I think we can close this, Observable promises made it into 2.2. |
VOTE HERE
The text was updated successfully, but these errors were encountered: