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

Support promises #418

Closed
13 of 32 tasks
coodoo opened this issue Jul 29, 2014 · 100 comments
Closed
13 of 32 tasks

Support promises #418

coodoo opened this issue Jul 29, 2014 · 100 comments

Comments

@coodoo
Copy link

coodoo commented Jul 29, 2014

LoopBack should provide Promise-enabled API out of the box. The plan is to use the approach outlined in nodejs/node#173.

Tasks


Below 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?

@wqli78
Copy link

wqli78 commented Aug 1, 2014

+1

2 similar comments
@glesage
Copy link

glesage commented Aug 3, 2014

+1

@wpjunior
Copy link

wpjunior commented Aug 3, 2014

+1

@offlinehacker
Copy link
Contributor

+1
On Aug 3, 2014 12:41 PM, "Wilson Júnior" notifications@github.com wrote:

+1


Reply to this email directly or view it on GitHub
#418 (comment).

@nowherenearithaca
Copy link

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.

@raymondfeng
Copy link
Member

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.

@partap
Copy link

partap commented Nov 18, 2014

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...

@partap
Copy link

partap commented Nov 18, 2014

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: "joe@schmoe.com"
| |  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? :)

@raymondfeng
Copy link
Member

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 ____accessTokens methods are added to the User.prototype for legacy remoting purposes. These methods will probably be removed from the model prototype in the future.

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 notifications@github.com 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: "joe@schmoe.com"
| | 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 #418 (comment).

@partap
Copy link

partap commented Nov 19, 2014

Hmm...ok, so if I understand correctly, all the properties and methods in models are defined using Object.defineProperty()?

  1. I can kind of see the advantage in using this technique to define some properties, since you are transparently calling a function instead of just returning a value (eg: something like string.length, or anything that might require computation or might be cached or memoized). I'm curious, though, what is the advantage of that methodology for defining methods, as opposed to something like 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? :)
  2. Where are the relations methods defined? I did a search for defineProperty and it seems to be used in the code a lot...
  3. Is there a way to enumerate properties and methods defined using defineProperty()?
  4. I'm wondering if I could figure out some way to promisify the relations methods at boot time. 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... @coodoo's trick uses 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.
  5. The model definition process seems pretty complicated and scattered throughout several modules. Would it be possible for a walkthrough of the major steps so one could trace it through the code? It seems that most models have a ton of mixins, and while many of these classes are documented, there doesn't seem to be any comprehensive explanation of what classes will end up in your finished model object and why (Model, PersistedModel, DataAccessObject, Validatable, etc...). For more complete documentation, it might be nice to make a note in the API documentation for each class just what it is used for. Why would I use it? Is it standalone? mixin for a Model? mixin for a DataSource? Subclass of something? Is it automatically included in every single model? Conversely, the docs for major developer-facing classes should list any builtin subclasses and mixins and what module in which they are defined. e.g.: 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.

@bajtos
Copy link
Member

bajtos commented Dec 18, 2014

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 nodejs/node#173. IMO it would be best to apply the same approach in loopback, as it involves only about 2-3 changed lines per function.

@bajtos bajtos added the feature label Dec 18, 2014
dotlouis added a commit to dotlouis/StudLoop that referenced this issue Dec 27, 2014
- Added https://github.com/petkaantonov/bluebird
- All methods now works with promises.
Remote methods get called like usual thanks to the awesome .nodeify()
- Required modules can be "promisified" with .promisifyAll(module);
- Loopback Models can be "promisified" (see
strongloop/loopback#418)

Result: cleaner, flatter, easier to understand code, better error
handling. see
http://spion.github.io/posts/why-i-am-switching-to-promises.html

Why Bluebird ? see
http://www.reddit.com/r/javascript/comments/1r3hwa/which_javascript_prom
ise_library_is_the_best_in/
@alFReD-NSH
Copy link
Contributor

Adding on, what you guys have said, it'd also be nice if remote methods could return promises.

@slawo
Copy link

slawo commented Dec 31, 2014

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.

@BerkeleyTrue
Copy link
Contributor

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.

dotlouis added a commit to dotlouis/StudLoop that referenced this issue Jan 14, 2015
- Added https://github.com/petkaantonov/bluebird
- All methods now works with promises.
Remote methods get called like usual thanks to the awesome .nodeify()
- Required modules can be "promisified" with .promisifyAll(module);
- Loopback Models can be "promisified" (see
strongloop/loopback#418)

Result: cleaner, flatter, easier to understand code, better error
handling. see
http://spion.github.io/posts/why-i-am-switching-to-promises.html

Why Bluebird ? see
http://www.reddit.com/r/javascript/comments/1r3hwa/which_javascript_prom
ise_library_is_the_best_in/
@ulion
Copy link
Contributor

ulion commented Feb 1, 2015

@partap so the promises for model relationship still not work by the bluebird plan?

@shlomiassaf
Copy link

+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.

@pandaiolo
Copy link
Contributor

I have wrapped everything in bluebird -Async promises, If that can influence the roadmap :-)

@mrfelton
Copy link
Contributor

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?

@PaddyMann
Copy link

Promisifying custom validators in loopback-datasource-juggler is missing from the checklist.

I've submitted a PR here: loopbackio/loopback-datasource-juggler#1229

Hoping to get your input @bajtos on what's needed on the testing front.

@bajtos
Copy link
Member

bajtos commented Jan 19, 2017

Promisifying custom validators in loopback-datasource-juggler is missing from the checklist.

Added to the checklist. Thank you for the pull request too!

@anodynos
Copy link

anodynos commented Mar 3, 2017

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 snowyu/loopback-component-auto-migrate.js#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.

@bajtos
Copy link
Member

bajtos commented Mar 3, 2017

@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?

@anodynos
Copy link

anodynos commented Mar 3, 2017

Thanks @bajtos - I opened it here #3248

@the-freshlord
Copy link

the-freshlord commented Jun 1, 2017

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.

@beeman
Copy link
Contributor

beeman commented Jun 1, 2017

@eman6576 that should work just fine. Can you post a reproduction of it?

@stale
Copy link

stale bot commented Aug 23, 2017

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.

@bajtos
Copy link
Member

bajtos commented Aug 23, 2017

@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.

@stale
Copy link

stale bot commented Oct 22, 2017

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.

@stale stale bot added the stale label Oct 22, 2017
@stale
Copy link

stale bot commented Nov 5, 2017

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.

@stale stale bot closed this as completed Nov 5, 2017
@bajtos bajtos reopened this Feb 9, 2018
@stale
Copy link

stale bot commented Apr 10, 2018

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.

@stale stale bot added the stale label Apr 10, 2018
@stale
Copy link

stale bot commented Apr 24, 2018

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.

@stale stale bot closed this as completed Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests