LoopBack should provide Promise-enabled API out of the box. The plan is to use the approach outlined in https://github.com/iojs/io.js/pull/173.
Tasks
automigrate, autoupdate, discover*, etc.app.listen@promise annotation https://github.com/strongloop-internal/strong-docs/issues/63@promise attributeBelow is the original proposal from @coodoo.
I've been playing around with bluebird's promisifyAll() for a while, ended up I found it just take a couple of line to wrap the model and make it become async-compatible.
See code below:
var Promise = require("bluebird");
module.exports = function(ResetKey) {
// once a model is attached to the data source
ResetKey.on('dataSourceAttached', function( obj ){
// wrap the whole model in Promise
// but we need to avoid 'validate' method
ResetKey = Promise.promisifyAll( ResetKey, {filter: function(name, func, target){
return !( name == 'validate');
}} );
})
}
Once the model is wrapped, we can use Promise syntax instead of callbacks:
// old way - callbacks
user.save(function(err, result){...})
// new way - promise
user.saveAsync()
.then(function(result){...})
.catch(function(err){...})
Ain't that lovely? :)
Of course it would be even better if all those happened higher up in the chain and become default.
Thought?
+1
+1
+1
+1
On Aug 3, 2014 12:41 PM, "Wilson Júnior" [email protected] wrote:
+1
—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-50987221.
This might be of interest to folks - talk on InfoQ about large nodejs project at kixeye where Paul Hill discusses why they liked Promises - http://www.infoq.com/presentations/kixeye-rest-api. While there is a _ton_ of useful info in this talk, he specifically discusses Promises (they used Bluebird) starting at about 21:50. Besides the syntactic sugar/readability, he highlights how there is an "implicit try-catch" that allows centralizing error handling/logging. He also shows a code snippet (25:51) for how they did this that might be a useful pattern to consider including in the docs at some point, if possible.
Thank you all for the input. We're building a list of roadmap items at https://github.com/strongloop/loopback/wiki/Roadmap-brainstorming. 'Promise' is on the list too. Maybe we should move it to a gist so that community ideas can be accepted via PRs.
I've actually been using something similar...after boot(app), insert:
Promise = require 'bluebird'
bbutil = require 'bluebird/js/main/util'
for model, p of app.models
Promise.promisifyAll p, filter: (name, func, target) ->
return bbutil.isIdentifier(name) &&
!bbutil.isClass(func) &&
name.charAt(0) isnt '_' &&
name.indexOf('Async') is -1 &&
name isnt 'app'
...pardon my coffeescript :)
This will promisify all the app's models at once. I have a couple more items in my filter to exclude attributes we don't need to promisify, but it basically works the same.
The main problem is that you can't use the promises in a boot script or model init script, but I think @coodoo takes care of that with his solution above (thanks!). You can use @coodoo's method for complex models, and then use the app.models loop to take care of the rest (it's ok to run promisifyAll() twice on the same object with this filter.
The other problem I've found is that it doesn't work for the model relations. For example:
user.posts.createAsync(...)
will fail...I'm not sure how to go about promisifying those "virtual selectors"
I'm not sure how or where they are defined or attached...
I was able to pause in the debugger and drill down a bit, but I'm still confused. There's some sort of convention for defining these methods that I'm not familiar with.
I'm looking at a user instance in the debugger...I'll try to asciify it :
user
| ModelConstructor
| __cachedRelations: Object
| __dataSource: Object
| |__data: Object
| | email: "[email protected]"
| | id: ObjectId
| | ...
| ...
| |__proto__: ModelConstructor
| | __count__accessTokens: function (cb) {
| | __create__accessTokens: function () {
| | __delete__accessTokens: function () {
| | ...
So if I understand this at all, it seems that anytime I access a property or method of a model instance, there's some javascript magic going on...user.id gets translated to user.__data.id, user.accessTokens.count() calls user.__count__accessTokens(), etc.
I saw similarly named methods in the lb-services.js file generated for angularjs...
...What is this magic and how does it work? :)
The methods for relations are added to the model as properties with getters. For example, user.accessTokens will return a function bound to the user instance. The function itself also has other methods such as ‘create’, ‘count’, and ‘delete’. Please note the __
Thanks,
Raymond Feng
Co-Founder and Architect @ StrongLoop, Inc.
StrongLoop http://strongloop.com/ makes it easy to develop APIs http://strongloop.com/mobile-application-development/loopback/ in Node, plus get DevOps capabilities http://strongloop.com/node-js-performance/strongops/ like monitoring, debugging and clustering.
On Nov 18, 2014, at 3:58 PM, Partap Davis [email protected] wrote:
I was able to pause in the debugger and drill down a bit, but I'm still confused. There's some sort of convention for defining these methods that I'm not familiar with.
I'm looking at a user instance in the debugger...I'll try to asciify it :
user
| ModelConstructor
| cachedRelations: Object
| __dataSource: Object
| |__data: Object
| | email: "[email protected]"
| | id: ObjectId
| | ...
| ...
| |__proto: ModelConstructor
| | __count__accessTokens: function (cb) {
| | __create__accessTokens: function () {
| | __delete__accessTokens: function () {
| | ...
So if I understand this at all, it seems that anytime I access a property or method of a model instance, there's some javascript magic going on...user.id gets translated to user.__data.id, user.accessTokens.count() calls user.__count__accessTokens(), etc.I saw similarly named methods in the lb-services.js file generated for angularjs...
...What is this magic and how does it work? :)
—
Reply to this email directly or view it on GitHub https://github.com/strongloop/loopback/issues/418#issuecomment-63568968.
Hmm...ok, so if I understand correctly, all the properties and methods in models are defined using Object.defineProperty()?
model.someMethod = someFunction.bind(model) or Model.prototype.someMethod = someFunction? It makes it hard to determine what the object's actual API is (although that might just be a limitation of the debugging tools?). I've been using javascript for years, but never found the need to use a getter. What magic goodness am I missing out on? :)defineProperty()?dataSourceAttached ... I see there is also an attached event. Are there new events added between the 2 events? Would it be possible to add an application-wide event in the boot sequence...something like app.model.on('modelDefined') that would pass in the model name and constructor function for every model defined at boot time. Obviously it couldn't be done in the general case because any time in the app's lifetime I can extend a model or define a new one, but it _seems_ like it would be possible during boot, since everything is defined from static resources.loopback-datasource-juggler _sounds_ like a behind-the-scenes type of module for backend connectivity, but looks like it actually defines a lot of core model behavior that I would have had no idea to look for there...Hmm...sorry about so many tangential questions...I got started and couldn't stop, hehe. Maybe we should move this thread into the google group? ...although, tbh, I rather prefer the markdown support here on github.
My idea is that, since there are different promise libraries available (I used to use Q, and now have transitioned to Bluebird), it might be a better idea to allow the user to use whatever promise library they want rather than make strongloop dependent on a single library...
+1
Note that Node v0.12/io.js 1.0 will ship a native implementation of Promises.
Thus a possible solution for LoopBack is to use the global Promise object in v0.12/1.0 and let the user supply whatever implementation he likes in Node v0.10 via global.Promise=require('bluebird');
I started to look into ways how to add promises to Node core API in https://github.com/iojs/io.js/pull/173. IMO it would be best to apply the same approach in loopback, as it involves only about 2-3 changed lines per function.
Adding on, what you guys have said, it'd also be nice if remote methods could return promises.
Actually adding promises to all loopback calls and all LoopBack Sdk Angular functions would help greatly unifying the API, at least on the callback front.
The loopback angular sdk already returns promises. It is built out on ngResource. For instance, User.logout().$promise.then();
It would be nice to get promises built in to the query syntax as well.
@partap so the promises for model relationship still not work by the bluebird plan?
+1.
Could you please post a behavior schema for the future implementation of promises.
Since they are a solid future feature for JS we will see a lot of frameworks going towards them.
An example is CO which is Promise based now.
I would like to start using them but I would like my changes to be future proof.
Will you guys implement a bluebird type schema? (e.g "Async" suffix?)
Will you support it on the same function name and return a promise if CB is not supplied?
How is it going to be?
Thanks.
I have wrapped everything in bluebird -Async promises, If that can influence the roadmap :-)
This is awesome. Using the techniques here I've been able to use Bluebird promises for most of my loopback code. The only exception I have found so far is the model relation methods which was mentioned earlier in this thread. Just wondering if anyone has come up with a way to promisify the complete model, including the relationship methods?
I'd like to modify the current API to support both callback and promise based usage using the approach shown in https://github.com/iojs/io.js/pull/173. I have updated the issue description to match this new direction.
+1. You could just create a promise backed callback function to replace https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L127-L131.
Cool!
I just tried this and it seems to be working so far... I have to make sure
to return the promise in all the different code paths.
I'll add in some tests to make sure and if it looks good, I'll submit a
PR...
-partap
On Fri, Feb 13, 2015 at 9:36 AM, Raymond Feng [email protected]
wrote:
+1. You could just create a promise backed callback function to replace
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L127-L131
.—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-74282733.
@bajtos
Just to make it clear,
You choose to implement it in a way that will keep the current API similar.
If a callback argument is not supplied, a promise is returned?
var result = SampleModel.findById(id);
console.log(typeof result.then === 'function'); // true.
Yes... and if the function previously returned a value, I'm returning the
same value if a callback is supplied.
On Fri, Feb 13, 2015 at 4:18 PM, Shlomi Assaf [email protected]
wrote:
@bajtos https://github.com/bajtos
Just to make it clear,
You choose to implement it in a way that will keep the current API similar.
If a callback argument is not supplied, a promise is returned?—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-74344504.
@partap
The RelationDefinition object gets created once for every relation in every model.
However, the invoking function/object gets created dynamically every time a relation instance is requested.
You can see it Here
The function in the link above adds the relation property to a model`s prototype (e.g: Model.relatedModel)
Object.defineProperty(modelFrom.prototype, relationName, {
enumerable: true,
configurable: true,
get: function() {
var relation = new BelongsTo(definition, this);
var relationMethod = relation.related.bind(relation);
relationMethod.update = relation.update.bind(relation);
relationMethod.destroy = relation.destroy.bind(relation);
if (!polymorphic) {
relationMethod.create = relation.create.bind(relation);
relationMethod.build = relation.build.bind(relation);
relationMethod._targetClass = definition.modelTo.modelName;
}
return relationMethod;
}
});
The getter returns a new BelongsTo classs, the class has definitions defined a few lines above, the scope is always current to the model invoking it.
So, lets say we have a Order model with a belongsTo relation to a "Customer" model.
An instance of Order should have a "customer" property.
If we invoke it: orderInstance.customer() we will get a returned value from the cache (if exists).
If we supply a callback, we will get a Customer instance (this will set refresh = false).
Now, we know that the relation invoker is in Order.prototype.customer, however, as I said before, it is a new instance of BelongsTo class that get instantiated every time you call Order.prototype.customer or orderInstance.customer(...).
Even if its possible, it will be costly to start dynamically altering the return function to a Promise.
This has to be done multiple times (create, destroy, etc...).
The best chance is to monkey patch the BelongsTo class for every call it has (related, destroy, create, etc) See Here
Since the realtionMethod is glued to an instance of BelongsTo context, the behavior need to be tested in bluebird to make sure nothing get broken...
The RelationDefinition module exports all the classes that handle related calls, Link
Changing all Relation classes is a big task.
We need to make sure we change them but keep the same behavior when using classic callbacks as they do returns values.
@mrfelton
As you can see, IMO, the only solution is to alter the Relation classes in the source and pack it as a release, it is quite complicated, though possible, to monkey patch the modules.
:+1:
:+1:
Thanks, it looks like that is the crux of the problem.
With the original callback mod, it doesn't support relations, and without
that, it's essentially the same as just running Promise.promisifyAll() on
each model, which is my current promise solution.
On Sat, Feb 14, 2015 at 3:34 PM, Shlomi Assaf [email protected]
wrote:
@partap https://github.com/partap
The RelationDefinition object gets created once for every relation in
every model.
However, the invoking function/object gets created dynamically every time
a relation instance is requested.You can see it Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L1104
This function adds the relation property to a model`s prototype (e.g:
Model.relatedModel)Object.defineProperty(modelFrom.prototype, relationName, {
enumerable: true,
configurable: true,
get: function() {
var relation = new BelongsTo(definition, this);
var relationMethod = relation.related.bind(relation);
relationMethod.update = relation.update.bind(relation);
relationMethod.destroy = relation.destroy.bind(relation);
if (!polymorphic) {
relationMethod.create = relation.create.bind(relation);
relationMethod.build = relation.build.bind(relation);
relationMethod._targetClass = definition.modelTo.modelName;
}
return relationMethod;
}
});The getter returns a new BelongsTo classs, the class has definitions
defined a few lines above, the scope is always current to the model
invoking it.So, lets say we have a Order model with a belongsTo relation to a
"Customer" model.
An instance of Order should have a "customer" property.
If we invoke it: orderInstance.customer()' we will get a returned value
from the cache (if exists).
If we supply a callback, we will get aCustomer` instance (this will set
refresh = false).Now, we know that the relation invoker is in Order.prototype.customer,
however, as I said before, it is a new instance of BelongsTo class that
get instantiated every time to you Order.prototype.customer or
orderInstance.customer(...).Even if its possible, it will be costly to start dynamically altering the
return function to a Promise.
This has to be done multiple times (create, destroy, etc...).The best chance is to monkey patch the BelongsTo class for every call it
has (related, destroy, create, etc) See Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L368—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-74394930.
The relation methods have a few synchronous modes:
For backwards compatibility, I'll only return a promise if refresh is an
explicit boolean and no callback is supplied:
if (arguments.length === 1) {
...sound ok?
On Sun, Feb 15, 2015 at 2:04 PM, Partap Davis [email protected] wrote:
Thanks, it looks like that is the crux of the problem.
With the original callback mod, it doesn't support relations, and without
that, it's essentially the same as just running Promise.promisifyAll() on
each model, which is my current promise solution.On Sat, Feb 14, 2015 at 3:34 PM, Shlomi Assaf [email protected]
wrote:@partap https://github.com/partap
The RelationDefinition object gets created once for every relation in
every model.
However, the invoking function/object gets created dynamically every time
a relation instance is requested.You can see it Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L1104
This function adds the relation property to a model`s prototype (e.g:
Model.relatedModel)Object.defineProperty(modelFrom.prototype, relationName, {
enumerable: true,
configurable: true,
get: function() {
var relation = new BelongsTo(definition, this);
var relationMethod = relation.related.bind(relation);
relationMethod.update = relation.update.bind(relation);
relationMethod.destroy = relation.destroy.bind(relation);
if (!polymorphic) {
relationMethod.create = relation.create.bind(relation);
relationMethod.build = relation.build.bind(relation);
relationMethod._targetClass = definition.modelTo.modelName;
}
return relationMethod;
}
});The getter returns a new BelongsTo classs, the class has definitions
defined a few lines above, the scope is always current to the model
invoking it.So, lets say we have a Order model with a belongsTo relation to a
"Customer" model.
An instance of Order should have a "customer" property.
If we invoke it: orderInstance.customer()' we will get a returned value
from the cache (if exists).
If we supply a callback, we will get aCustomer` instance (this will set
refresh = false).Now, we know that the relation invoker is in Order.prototype.customer,
however, as I said before, it is a new instance of BelongsTo class that
get instantiated every time to you Order.prototype.customer or
orderInstance.customer(...).Even if its possible, it will be costly to start dynamically altering the
return function to a Promise.
This has to be done multiple times (create, destroy, etc...).The best chance is to monkey patch the BelongsTo class for every call it
has (related, destroy, create, etc) See Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L368—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-74394930
.
Ran into a hiccup while testing:
DAO.create() supports chaining, but I'm not sure how to handle it... I'm
returning the new data if a callback is supplied, otherwise the promise.
This passes the current test suite.
How is chaining expected to work? Should I continue to return the data
object or array even when using promises?
I could, for example, attach a $promise variable to the data, similar to
Angular's $resource service.
Would that be useful, or just clutter? I believe the only reason the
angular team did that instead of returning the pure promise is that there
was already lots of code written that relied on the resource object being
returned synchronously.
It seems like the only chaining in loopback is in create() though, and I'm
not sure how widespread this usage is, or even how useful it is,
considering we don't know if the creation is successful until the callback.
I'm only seeing the chaining used in one place in the test suite
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L170.
My code passes this test, but if the callback is omitted it will return a
promise instead of the data array. I think that's fine, but just wondering
if there were any objections.
On Sun, Feb 15, 2015 at 4:03 PM, Partap Davis [email protected] wrote:
The relation methods have a few synchronous modes:
- - order.customer(refresh, callback): Load the target model instance
asynchronously- - order.customer(customer): Synchronous setter of the target model
instance- - order.customer(): Synchronous getter of the target model instance
For backwards compatibility, I'll only return a promise if refresh is an
explicit boolean and no callback is supplied:if (arguments.length === 1) {
- if (typeof refresh === 'boolean') {
- params = utils.createPromiseCallback();
- } else {
params = refresh;
refresh = false;- }
...sound ok?
On Sun, Feb 15, 2015 at 2:04 PM, Partap Davis [email protected] wrote:
Thanks, it looks like that is the crux of the problem.
With the original callback mod, it doesn't support relations, and without
that, it's essentially the same as just running Promise.promisifyAll() on
each model, which is my current promise solution.On Sat, Feb 14, 2015 at 3:34 PM, Shlomi Assaf [email protected]
wrote:@partap https://github.com/partap
The RelationDefinition object gets created once for every relation in
every model.
However, the invoking function/object gets created dynamically every
time a relation instance is requested.You can see it Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L1104
This function adds the relation property to a model`s prototype (e.g:
Model.relatedModel)Object.defineProperty(modelFrom.prototype, relationName, {
enumerable: true,
configurable: true,
get: function() {
var relation = new BelongsTo(definition, this);
var relationMethod = relation.related.bind(relation);
relationMethod.update = relation.update.bind(relation);
relationMethod.destroy = relation.destroy.bind(relation);
if (!polymorphic) {
relationMethod.create = relation.create.bind(relation);
relationMethod.build = relation.build.bind(relation);
relationMethod._targetClass = definition.modelTo.modelName;
}
return relationMethod;
}
});The getter returns a new BelongsTo classs, the class has definitions
defined a few lines above, the scope is always current to the model
invoking it.So, lets say we have a Order model with a belongsTo relation to a
"Customer" model.
An instance of Order should have a "customer" property.
If we invoke it: orderInstance.customer()' we will get a returned value
from the cache (if exists).
If we supply a callback, we will get aCustomer` instance (this will set
refresh = false).Now, we know that the relation invoker is in Order.prototype.customer,
however, as I said before, it is a new instance of BelongsTo class that
get instantiated every time to you Order.prototype.customer or
orderInstance.customer(...).Even if its possible, it will be costly to start dynamically altering
the return function to a Promise.
This has to be done multiple times (create, destroy, etc...).The best chance is to monkey patch the BelongsTo class for every call
it has (related, destroy, create, etc) See Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L368—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-74394930
.
Another issue in create...
Apparently the batch creation allows "partial success"
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L159-L167...
if validation fails on one record, the others are still created and
returned in the callback.
Normally, node callbacks have an error as the first argument, and if it is
set, then the rest of the arguments are unused, which is great for mapping
to promises, which either succeed or fail in separate code paths.
The simple promise callback would fail here because any single error
triggers the error path...even though 2 of the 3 records were successfully
created.
There are several ways to handle this with promises... problem is that I
don't think they are standardized in Promises/A+.
Bluebird has settle(), Q has allSettled(), and both have incompatible
arguments passed to the handlers...there are probably other versions on
other libraries.
I guess simplest solution, in the case of a batch create, would be to aways
succeed, but pass the errors along with the results...
On Sun, Feb 15, 2015 at 5:47 PM, Partap Davis [email protected] wrote:
Ran into a hiccup while testing:
DAO.create() supports chaining, but I'm not sure how to handle it... I'm
returning the new data if a callback is supplied, otherwise the promise.
This passes the current test suite.How is chaining expected to work? Should I continue to return the data
object or array even when using promises?
I could, for example, attach a $promise variable to the data, similar to
Angular's $resource service.Would that be useful, or just clutter? I believe the only reason the
angular team did that instead of returning the pure promise is that there
was already lots of code written that relied on the resource object being
returned synchronously.It seems like the only chaining in loopback is in create() though, and I'm
not sure how widespread this usage is, or even how useful it is,
considering we don't know if the creation is successful until the callback.I'm only seeing the chaining used in one place in the test suite
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L170.
My code passes this test, but if the callback is omitted it will return a
promise instead of the data array. I think that's fine, but just wondering
if there were any objections.On Sun, Feb 15, 2015 at 4:03 PM, Partap Davis [email protected] wrote:
The relation methods have a few synchronous modes:
- - order.customer(refresh, callback): Load the target model instance
asynchronously- - order.customer(customer): Synchronous setter of the target model
instance- - order.customer(): Synchronous getter of the target model instance
For backwards compatibility, I'll only return a promise if refresh is an
explicit boolean and no callback is supplied:if (arguments.length === 1) {
- if (typeof refresh === 'boolean') {
- params = utils.createPromiseCallback();
- } else {
params = refresh;
refresh = false;- }
...sound ok?
On Sun, Feb 15, 2015 at 2:04 PM, Partap Davis [email protected]
wrote:Thanks, it looks like that is the crux of the problem.
With the original callback mod, it doesn't support relations, and
without that, it's essentially the same as just running
Promise.promisifyAll() on each model, which is my current promise solution.On Sat, Feb 14, 2015 at 3:34 PM, Shlomi Assaf [email protected]
wrote:@partap https://github.com/partap
The RelationDefinition object gets created once for every relation in
every model.
However, the invoking function/object gets created dynamically every
time a relation instance is requested.You can see it Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L1104
This function adds the relation property to a model`s prototype (e.g:
Model.relatedModel)Object.defineProperty(modelFrom.prototype, relationName, {
enumerable: true,
configurable: true,
get: function() {
var relation = new BelongsTo(definition, this);
var relationMethod = relation.related.bind(relation);
relationMethod.update = relation.update.bind(relation);
relationMethod.destroy = relation.destroy.bind(relation);
if (!polymorphic) {
relationMethod.create = relation.create.bind(relation);
relationMethod.build = relation.build.bind(relation);
relationMethod._targetClass = definition.modelTo.modelName;
}
return relationMethod;
}
});The getter returns a new BelongsTo classs, the class has definitions
defined a few lines above, the scope is always current to the model
invoking it.So, lets say we have a Order model with a belongsTo relation to a
"Customer" model.
An instance of Order should have a "customer" property.
If we invoke it: orderInstance.customer()' we will get a returned
value from the cache (if exists).
If we supply a callback, we will get aCustomer` instance (this will
set refresh = false).Now, we know that the relation invoker is in Order.prototype.customer,
however, as I said before, it is a new instance of BelongsTo class
that get instantiated every time to you Order.prototype.customer or
orderInstance.customer(...).Even if its possible, it will be costly to start dynamically altering
the return function to a Promise.
This has to be done multiple times (create, destroy, etc...).The best chance is to monkey patch the BelongsTo class for every call
it has (related, destroy, create, etc) See Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L368—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-74394930
.
I submitted a PR here:
https://github.com/strongloop/loopback-datasource-juggler/pull/447
There is still a lot of testing to do with related models, but the
manipulation tests seem fine. For the most part, I just went through the
existing tests and duplicated them using promises where applicable.
If anybody wants to help with the tests for related models, I'd appreciate
it...there are a _lot_ of them!
-partap
On Sun, Feb 15, 2015 at 6:25 PM, Partap Davis [email protected] wrote:
Another issue in create...
Apparently the batch creation allows "partial success"
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L159-L167...
if validation fails on one record, the others are still created and
returned in the callback.Normally, node callbacks have an error as the first argument, and if it is
set, then the rest of the arguments are unused, which is great for mapping
to promises, which either succeed or fail in separate code paths.The simple promise callback would fail here because any single error
triggers the error path...even though 2 of the 3 records were successfully
created.There are several ways to handle this with promises... problem is that I
don't think they are standardized in Promises/A+.Bluebird has settle(), Q has allSettled(), and both have incompatible
arguments passed to the handlers...there are probably other versions on
other libraries.I guess simplest solution, in the case of a batch create, would be to
aways succeed, but pass the errors along with the results...On Sun, Feb 15, 2015 at 5:47 PM, Partap Davis [email protected] wrote:
Ran into a hiccup while testing:
DAO.create() supports chaining, but I'm not sure how to handle it... I'm
returning the new data if a callback is supplied, otherwise the promise.
This passes the current test suite.How is chaining expected to work? Should I continue to return the data
object or array even when using promises?
I could, for example, attach a $promise variable to the data, similar to
Angular's $resource service.Would that be useful, or just clutter? I believe the only reason the
angular team did that instead of returning the pure promise is that there
was already lots of code written that relied on the resource object being
returned synchronously.It seems like the only chaining in loopback is in create() though, and
I'm not sure how widespread this usage is, or even how useful it is,
considering we don't know if the creation is successful until the callback.I'm only seeing the chaining used in one place in the test suite
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L170.
My code passes this test, but if the callback is omitted it will return a
promise instead of the data array. I think that's fine, but just wondering
if there were any objections.On Sun, Feb 15, 2015 at 4:03 PM, Partap Davis [email protected]
wrote:The relation methods have a few synchronous modes:
- - order.customer(refresh, callback): Load the target model instance
asynchronously- - order.customer(customer): Synchronous setter of the target model
instance- - order.customer(): Synchronous getter of the target model instance
For backwards compatibility, I'll only return a promise if refresh is an
explicit boolean and no callback is supplied:if (arguments.length === 1) {
- if (typeof refresh === 'boolean') {
- params = utils.createPromiseCallback();
- } else {
params = refresh;
refresh = false;- }
...sound ok?
On Sun, Feb 15, 2015 at 2:04 PM, Partap Davis [email protected]
wrote:Thanks, it looks like that is the crux of the problem.
With the original callback mod, it doesn't support relations, and
without that, it's essentially the same as just running
Promise.promisifyAll() on each model, which is my current promise solution.On Sat, Feb 14, 2015 at 3:34 PM, Shlomi Assaf <[email protected]
wrote:
@partap https://github.com/partap
The RelationDefinition object gets created once for every relation in
every model.
However, the invoking function/object gets created dynamically every
time a relation instance is requested.You can see it Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L1104
This function adds the relation property to a model`s prototype (e.g:
Model.relatedModel)Object.defineProperty(modelFrom.prototype, relationName, {
enumerable: true,
configurable: true,
get: function() {
var relation = new BelongsTo(definition, this);
var relationMethod = relation.related.bind(relation);
relationMethod.update = relation.update.bind(relation);
relationMethod.destroy = relation.destroy.bind(relation);
if (!polymorphic) {
relationMethod.create = relation.create.bind(relation);
relationMethod.build = relation.build.bind(relation);
relationMethod._targetClass = definition.modelTo.modelName;
}
return relationMethod;
}
});The getter returns a new BelongsTo classs, the class has definitions
defined a few lines above, the scope is always current to the model
invoking it.So, lets say we have a Order model with a belongsTo relation to a
"Customer" model.
An instance of Order should have a "customer" property.
If we invoke it: orderInstance.customer()' we will get a returned
value from the cache (if exists).
If we supply a callback, we will get aCustomer` instance (this will
set refresh = false).Now, we know that the relation invoker is in Order.prototype.customer,
however, as I said before, it is a new instance of BelongsTo class
that get instantiated every time to you Order.prototype.customer or
orderInstance.customer(...).Even if its possible, it will be costly to start dynamically altering
the return function to a Promise.
This has to be done multiple times (create, destroy, etc...).The best chance is to monkey patch the BelongsTo class for every call
it has (related, destroy, create, etc) See Here
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/relation-definition.js#L368—
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/418#issuecomment-74394930
.
For backwards compatibility, I'll only return a promise if refresh is an
explicit boolean and no callback is supplied.
In my opinion, such rather unfriendly. Most people will forget the true argument and get undefined in the better case, or a cached object in the worse case. The code won't make it obvious where the mistake is, thus troubleshooting will be difficult.
It also violates the promise pattern where all promise functions should always return a promise, regardless of the argument values.
I would prefer to create a new method for getting the related data that would be purely async (callback/promise based). We already have order.customer.delete, the new method can be called order.customer.get.
DAO.create() supports chaining, but I'm not sure how to handle it... I'm
returning the new data if a callback is supplied, otherwise the promise.
This passes the current test suite.
I am ok with the solution you have described. @raymondfeng @ritch any objections?
In fact, I am proposing to change the create method to never return the created object, thus making the create method consistent with the rest of DAO methods. Since this will be a breaking change, it has to wait until 3.0.
I could, for example, attach a $promise variable to the data, similar to Angular's $resource service.
Please don't. $promise property is a PITA to use, other projects like restangular had learned the lesson and use a different approach.
Apparently the batch creation allows "partial success". If validation fails on one record, the others are still created and returned in the callback.
Let's keep this simple and disable the batch create in the promise mode for now. That way we can support the most common use case, and worry about the batch mode only after there is a user asking for it.
For backwards compatibility, I'll only return a promise if refresh is an
explicit boolean and no callback is supplied.In my opinion, such rather unfriendly. Most people will forget the true
argument and get undefined in the better case, or a cached object in the
worse case. The code won't make it obvious where the mistake is, thus
troubleshooting will be difficult.I agree... I've already forgotten it a few times writing the unit tests :-/
It also violates the promise pattern where all promise functions should
always return a promise, regardless of the argument values.Hear hear.
I would prefer to create a new method for getting the related data that
would be purely async (callback/promise based). We already have
order.customer.delete, the new method can be called order.customer.get.Ok... I wasn't sure what you guys would think about adding new methods to
the API, but that does sound like the best way to go about it, while
staying backwards compatible.
Let's keep this simple and disable the batch create in the promise mode for
now. That way we can support the most common use case, and worry about the
batch mode only after there is a user asking for it.
Will do...
I could, for example, attach a $promise variable to the data, similar to
Angular's $resource service.
Please don't. $promise property is a PITA to use, other projects like
restangular had learned the lesson and use a different approach.Hehe, it wasn't _my_ idea! :)
I actually did the first version of the patch for $resource a couple years
back... originally it just returned a promise, but they wanted to keep the
existing Resource return value for backward compatibility, so the promise
got tacked on as $q, and later, $promise.
At first I thought maybe they would change the API to purely promise-based
at the next version bump...now I'm just used to adding .$promise after
every $resource call...sigh
https://groups.google.com/forum/#!searchin/angular/Wanting$20$24resource$20to$20create$20resources$20that$20return$20$24q$20promises/angular/oK5HWEHoWvc/W3711wnqUv0J
https://groups.google.com/forum/#!searchin/angular/Easiest$20way$20to$20use$20promises$2C$20$24q$2C$20and$20$24resource/angular/jKU8Xcc7aFg/Xen4V8SaIoYJ
Please don't. $promise property is a PITA to use
Haha! so true
Hi, is the model method can do promise at this time? I still using callback right now.
@ltlikesstudy Hi, is the model method can do promise at this time?
You can use promises for CRUD methods, see https://github.com/partap/loopback-datasource-juggler/commit/1e6c453191f49c0f4a25fec2d694856c0cdf622a. Promises for relations methods are in progress, see strongloop/loopback-datasource-juggler#452
Anybody having some issues with node-redis losing cls namespace? on .then.
Seems like something like https://github.com/TimBeyer/cls-bluebird would be required to make cls work better with node-redis and other packages.
@seriousben That issue was fixed at version 2.8.8, what version are you using?
@alFReD-NSH here is what I am using right now. This seems to be related to node-redis specifically.
├─┬ [email protected]
│ └─┬ [email protected]
│ └── [email protected]
└── [email protected]
@bajtos I don't seem to be able to use promises on CRUD on my models at the moment. I'm using a data source with the remote connector though. I guess that isn't supported yet?
@bajtos I don't seem to be able to use promises on CRUD on my models at the moment. I'm using a data source with the remote connector though. I guess that isn't supported yet?
Yes, the remote connector does not support promises AFAIK. I have added a new task to the todo list in the issue description. Can you contribute the implementation yourself?
Hurrah, the patch for relation methods has landed on juggler's master: https://github.com/strongloop/loopback-datasource-juggler/pull/452 Kudos to @partap!
@bajtos - So the CRUD methods for models and relatedModels now use promises ... but something like User.login() still does not ... correct?
@pulkitsinghal So the CRUD methods for models and relatedModels now use promises ... but something like
User.login()still does not ... correct?
Yes, that's a correct description of the current status.
@OwenBrotherwood
The issue seems to have a long lived nature, with no clear definition of when it can be closed.
See the list of tasks in the issue description.
Is there an issue tracking Promise in doc?
then I can delete this comment to remove noise from this issue
Opened #1409
Late, but +1
+1
+1
You may also want to check out this library I wrote called promise-breaker. Basically, if you have existing code which uses callbacks:
exports.save = function(param1, param2, cb) {...}
You do this:
pb = require('promise-breaker');
exports.save = pb.make(function(param1, param2, cb) {...});
Which converts save into a function which accepts either a callback or, if none is provided, returns a Promise. Super tiny library, works with global.Promise if it's defined (so you can use your Promise provider of choice), and it doesn't murder the .length of a function. Also, promise-breaker has a callFn and an applyFn for calling into functions when you don't know if they're expecting a callback or if they're going to return a Promise - handy for calling into user-defined remote methods, for example.
+1
+1 Definitely promises are necessary.
Just a heads up that I've started documentation: https://docs.strongloop.com/display/LB/Using+promises
At this point, it's just a high-level introduction and a bare start. I expect to expand on this quite a bit.
Please feel free to improve on it if you are interested.
Additionally, there are separate issues for:
There's no mention of email connector or email model here. Don't forget about this one please.
@alFReD-NSH good point, I have added this item to the list of tasks. Would you mind contributing this improvement yourself?
Very tied up at the moment. Maybe later if I have time. :\ Not a priority on my list at the moment.
+1
+1
+1
Seems like _hasAndBelongsToMany_ relations are not promisified neither.
Example
Article (extended from PersistedModel)
"relations": {
"contributors": {
"type": "hasAndBelongsToMany",
"model": "Author"
}
}
Author (extended from User)
"relations": {
"contributions": {
"type": "hasAndBelongsToMany",
"model": "Article"
}
}
author.contributions().then() // throw an error: Cannot read property 'then' of undefined.
Promise.resolve(author.contributions) // returns always ``undefined``
/* works */
author.contributions(function(err, articles){
console.log(articles);
});
Maybe i'm missing something...
@julien-sarazin I don't think any of the relationship methods are promisified directly, although they do support promises via the getAsync method which is promisified. eg
author.contributions.getAsync().then()
You're totally right. Thank you!
Also needs to support Model.afterRemote etc
+1
+1
+1 @bajtos Any updates on this? I see a lot of merged code. Docs don't reflect it though
Docs are lagging a bit, but we're going to start working on them. We have tracking issues per https://github.com/strongloop/loopback/issues/418#issuecomment-135494570. One issue is that we need to update our API doc code to allow for both callback and promise API.
@gaurav21r Note that we are using an internal repo to generate API docs, and we have an issue to enhance it to support the @promise annotation: https://github.com/strongloop-internal/strong-docs/issues/63 (this is not currently visible externally). I don't see any reason why this repo should be private, though.... I'm going to ask that we make it public.
+1
Why doesn't the general PersistedModel.create return a promise like the built in models?
+1
+1
If you're interested in this issue, instead of commenting "+1", please hit "Subscribe" on the right of this page. You'll get updates about this issue, and it's clear how many people are interested.
@crandmck Any luck with making it open? I am sure community participation will only help! Can't think of much IP in the doc engine!
Any luck with making it open?
@gaurav21r It's in the plan, and everyone agrees it's a good idea. Just need to sort out the details. There's a bunch of logistics around SL become part of IBM. The good news is that we've _just_ landed support for the @promise annotation in strong-docs, so we can now start updating the doc comments, and that will soon be reflected in the API docs.
FYI @davidcheung has taken on several tasks to update the API doc comments for promise support. I'll let him comment on the details, but I hope and expect that the work can be completed very soon.
Let's keep this simple and disable the batch create in the promise mode for
now. That way we can support the most common use case, and worry about the
batch mode only after there is a user asking for it.Will do...
Is this still true? It seems to be. I get:
TypeError: XXX.app.models.YYY.create(...).then is not a function
at /.../zzz.js:42:6
@mmoutenot Why doesn't the general PersistedModel.create return a promise like the built in models?
Ah, that's an oversight, would you mind to submit a pull request fixing the problem?
@jbcpollak could you please open a new issue to discuss batch create with promises?
I'm not sure if this is the right place to report this, but Model.save() does not return a Promise. If it matters, my model is using a remote connector.
I am using these versions:
"loopback": "=2.28.0",
"loopback-boot": "=2.18.1",
"loopback-component-explorer": "=2.5.0",
"loopback-connector-remote": "=1.3.1",
"loopback-datasource-juggler": "=2.46.0",
This will be really wonderful when it happens, +1ing this so hard
Promise support for remoting is pretty confusing at the moment (latest stable, i.e. 2.29.1). The following works:
app.remotes().after('UserAccount.login', function() {
// Do something here
return Promise.resolve(); // Resumes the chain
});
But this (which I thought was just a different way of achieving the same thing) hangs every login request. Took some time to figure out that it was because of the promise being ignored.
UserAccount.afterRemote('login', function() {
// Do something here
return Promise.resolve(); // Stops the chain
});
But then with an operation hook, which is not technically remoting but syntactically very similar to the previous example, returning a promise actually does work.
UserAccount.observe('after save', function() {
// Do something here
return Promise.resolve(); // Resumes the chain
});
+1 guys ... very support this features...
But this (which I thought was just a different way of achieving the same thing) hangs every login request. Took some time to figure out that it was because of the promise being ignored.
UserAccount.afterRemote('login', function() {
// Do something here
return Promise.resolve(); // Stops the chain
});
This is a bug in loopback. See the implementation of Model.before/after|remote/Error: https://github.com/strongloop/loopback/blob/6752dd3af3dd1ced800f4175317a786cedb3998e/lib/model.js#L193-L221
@tvdstaaij would you mind contributing the fix and sending a pull request? Please mention my GH handle (@bajtos) in the pull request description, so that I notice it in the flood of other notifications I am receiving.
@vroudge @sayasemut and everybody else subscribed to this thread:
While we are definitely in favour of making all LoopBack APIs promise-enabled, our bandwidth is limited and there are many other, more complex issues needing our attention.
We are encouraging everybody in the community to help us with making this happen. Adding support for promises is a pretty easy task, take a look at prior pull requests to see the pattern to use.
@bajtos i see your point, okay let see what i can do ..
Just curious, how relevant is this thread to Loopback 3.0? Does it still only have promises available in some areas? I remember seeing something in the loopback 3 announcement (https://strongloop.com/strongblog/coming-soon-loopback-3-0/) that bluebird would be a part of everything.
@jcolemorrison
I remember seeing something in the loopback 3 announcement (https://strongloop.com/strongblog/coming-soon-loopback-3-0/) that bluebird would be a part of everything.
When a LoopBack 3.x method returns a promise, then it's a promise created by Bluebird.
Does it still only have promises available in some areas?
Yes, we haven't enough bandwidth to promisify all APIs. See the checklist in the issue description at the top for a list of places that need to be promisified yet.
Contributions are welcome!
Promisifying custom validators in loopback-datasource-juggler is missing from the checklist.
I've submitted a PR here: https://github.com/strongloop/loopback-datasource-juggler/pull/1229
Hoping to get your input @bajtos on what's needed on the testing front.
Promisifying custom validators in loopback-datasource-juggler is missing from the checklist.
Added to the checklist. Thank you for the pull request too!
It would be great if we could add support for a Promise to be returned for each loopback-component-XXX loaded status + more.
This will allow us to decide whether to perform a task or not, or even disable parts of the API's functionality, wait before running tests etc.
See for example this workaround beginning of a nice and simple loopback <--> components contract for auto-migration https://github.com/snowyu/loopback-component-auto-migrate.js/pull/4
In summary we need a (lightweight) contract so components can be looked up as app.components['superPlugin'], and provide some rudimentary lifecycle (then) hooks (eg loaded, stopped etc) and even have a custom exported object.
@anodynos thank you for the comment. Your request makes sense to me, however it's rather off topic in this thread, which is focused on modifying existing APIs to support dual callback/promise. Could you please open a new issue to discuss component registration/discovery?
Thanks @bajtos - I opened it here https://github.com/strongloop/loopback/issues/3248
I noticed that when using promises with instance methods of PersistedModel, I get this error:
{ error: syntax error at or near "WHERE"
at Connection.parseE (/Users/Emanuel/Desktop/SilverLogic/picks-loopback/node_modules/pg/lib/connection.js:569:11)
at Connection.parseMessage (/Users/Emanuel/Desktop/SilverLogic/picks-loopback/node_modules/pg/lib/connection.js:396:17)
at Socket.<anonymous> (/Users/Emanuel/Desktop/SilverLogic/picks-loopback/node_modules/pg/lib/connection.js:132:22)
at emitOne (events.js:96:13)
at Socket.emit (events.js:188:7)
at readableAddChunk (_stream_readable.js:176:18)
at Socket.Readable.push (_stream_readable.js:134:10)
at TCP.onread (net.js:548:20)
name: 'error',
length: 87,
severity: 'ERROR',
code: '42601',
detail: undefined,
hint: undefined,
position: '28',
internalPosition: undefined,
internalQuery: undefined,
where: undefined,
schema: undefined,
table: undefined,
column: undefined,
dataType: undefined,
constraint: undefined,
file: 'scan.l',
line: '1081',
routine: 'scanner_yyerror' }
So perhaps support for instance methods on PersistedModel doesn't exist yet. I tried this when calling updatingAttributes and save.
@eman6576 that should work just fine. Can you post a reproduction of it?
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@kjdelisle @raymondfeng I think we may need to address this issue as part of our work on LoopBackNext. At least the parts in juggler (e.g. scope methods), because we are reusing juggler in LoopBackNext so far.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.
Most helpful comment
This will be really wonderful when it happens, +1ing this so hard