Related Models Promisification #452

partap · 2015-02-18T05:21:13Z

Rebased and isolated related models promise code from #447

The tests currently rely on promise-capable DAO from #451, but the code itself should work separately.

slnode · 2015-02-18T05:21:14Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

partap · 2015-02-18T22:58:01Z

@bajtos
I've updated the relations to use, e.g., customer.orders.get() for promise-based retrieval to avoid the awkwardness of customer.orders(true)

partap · 2015-02-18T23:02:43Z

@bajtos
Rather than rewrite ~70 tests in relations.test.js to only use promises for relations and not dao, I suggest copying the dao.js file from the promises-dao into the working tree here to get the tests to run...

partap · 2015-02-21T22:04:04Z

@bajtos
There is a ton of trailing whitespace in relation-definition.js.
I've been dancing around it with my commits... Could you take care of it in the master branch?

bajtos · 2015-02-24T11:26:27Z

There is a ton of trailing whitespace in relation-definition.js. I've been dancing around it with my commits... Could you take care of it in the master branch?

Done.

bajtos · 2015-02-24T11:28:37Z

lib/relation-definition.js

@@ -1276,8 +1313,12 @@ BelongsTo.prototype.related = function (refresh, params) {
  var scopeQuery = null;

  if (arguments.length === 1) {
+    if (typeof refresh === 'boolean') {
+      params = utils.createPromiseCallback();
+    } else {


This makes Product.categories(true) return a promise, correct? I thought we agreed that Product.categories() will not be promise-enabled at all. I am proposing to revert all changes made in the related() method.

Oops.
Yeah, I added the get() methods but didn't remove promises from the related() methods.
...although it doesn't really hurt anything, I guess...

partap · 2015-02-25T02:21:34Z

Oh snap... removing all promise support from related() breaks get()

partap · 2015-02-25T02:28:21Z

...and removing the refresh arg from get also breaks it... it could be a filter condition.

bajtos · 2015-03-04T13:18:21Z

@partap what's the status of this PR, is it ready for final review?

partap · 2015-03-04T19:33:39Z

@bajtos I believe so.

partap · 2015-03-04T19:35:59Z

@bajtos
Rebased on latest master with DAO Promise support now.
I redid the get methods to work the same way as related, rather than forcing refresh=true...it's a little messy with the argument resolution because of the arguments checks, but I think it would probably be better to change the default for refresh in related than in get, and that would be beyond the scope of this PR.

bajtos · 2015-03-09T15:32:17Z

I redid the get methods to work the same way as related, rather than forcing refresh=true...it's a little messy with the argument resolution because of the arguments checks, but I think it would probably be better to change the default for refresh in related than in get, and that would be beyond the scope of this PR.

The "related()" API is messy, confusing and broken, I have intentionally asked to use a better approach in the new "get()" method. See the discussion in strongloop/loopback#256 for more details.

Also boolean-based APIs are really bad, it's very difficult to tell what is the code Product.categories(true) doing - is it saying refresh=true, useCache=true, or something completely else?

bajtos · 2015-03-09T15:33:53Z

If we want to provide a function for getting the data from the cache first, then let's can add getCached to make the distinction clear. IMO it can be left out of scope of this PR.

bajtos · 2015-03-09T15:34:01Z

@slnode ok to test

When a callback is omitted from a method on a model relation that supports promises, return that promise. This includes all the standard DAO methods, as well as any user-defined methods that return promises. e.g.: mylist.todos.create({name: 'Item 1'}) // returns Promise This API will use native ES6 promises if available. If not available, or to force the use of another Promise library, you must assign the global.Promise object. e.g.: global.Promise = require('bluebird') Relations affected: - BelongsTo - HasOne - HasMany - HasManyThrough - HasAndBelongsToMany - ReferencesMany - EmbedsOne Exceptions: The EmbedsMany relation has not been promisified, because most of the methods return synchronous values. The base relation getter method [e.g.: mylist.todos()] has not been promisified, due to its default caching behavior. New Methods: - getAsync(condition, cb) A new method "getAsync()" has been added to all relations except EmbedsMany, which always fetches from the datasource rather than from the cache. It takes an optional "where" condition (except for HasOne and BelongsTo) and an optional callback. If the callback is omitted, a Promise is returned.

partap · 2015-03-30T22:28:35Z

@bajtos It looks ok now.

bajtos · 2015-04-01T06:55:54Z

lib/scope.js

+        } else if (cb === undefined && typeof cond !== 'function') {
+          cb = utils.createPromiseCallback();
+        }
+        definition.related(self, f._scope, cond, cb);


I think this code does not correctly handle the variant getAsync(mycb).

It will call related(self, f._scope, mycb, undefined), and because related is checking arguments.length, it will parse the arguments incorrectly.

I think this is the correct implementation:

if (cb === undefined) { if (cond === undefined) { // getAsync() cb = utils.createPromiseCallback(); cond = true; } else if (typeof cond !== 'function') { // getAsync({where:{}}) cb = utils.createPromiseCallback(); } else { // getAsync(function(){}) cb = cond; cond = true; } }

Thoughts?

bajtos · 2015-04-01T07:00:15Z

@partap I found an issue the proposed implementation of scope.getAsync, see my comment above. I can apply the proposed fix myself to save you time, if you like.

partap · 2015-04-03T18:41:29Z

@bajtos
Good catch, thanks!

That case wasn't covered by the tests so I added one... your fix works.

partap · 2015-04-03T18:48:39Z

@bajtos
Not sure why is says failing check now... All tests pass on my end.

Hmm. It appears there is a request from "Jenkins" to allow read-write access to all my public and private repos. I can't imagine how that would be necessary.

(edit: sorry for the delay, my macbook has been in the shop the last few days)

bajtos · 2015-04-07T08:09:21Z

@slnode test please

bajtos · 2015-04-07T08:10:45Z

Not sure why is says failing check now... All tests pass on my end.

The build slaves were not working correctly. I have restarted the build.

Hmm. It appears there is a request from "Jenkins" to allow read-write access to all my public and private repos. I can't imagine how that would be necessary.

Yeah, I agree that should not be required. Can you @rmg comment on this please?

bajtos · 2015-04-07T08:12:53Z

@slnode test please

bajtos · 2015-04-07T09:39:33Z

The CI is broken, I have verified the implementation locally on my machine.

Promisify model relation methods

bajtos · 2015-04-07T09:43:24Z

Landed via 2bdcce0, thank you for this valuable contribution!

esco · 2015-04-07T11:24:25Z

👍

rmg · 2015-04-07T14:51:55Z

Jenkins requires that level of access so that it can verify that you are part of the strongloop org and that you have commit access to the repos it is validating you against -- It is not a public CI.

michael-lumley · 2016-04-05T10:35:40Z

Is there any documentation on this, and the correct syntax for implementing it?

superkhau · 2016-04-05T17:27:18Z

Is there any documentation on this, and the correct syntax for implementing it?

@crandmck ^

crandmck · 2016-04-07T00:15:42Z

No, unfortunately I haven't had time to document use of the promise API at all. There are lots of open issues for the existing docs, so it would have to be prioritized relative to those.... Before I can start on that, though, it would be very helpful to have a few "promise-ized" sample apps. @superkhau perhaps you could help in that area? For example (maybe) loopback-getting-started-promise, etc.

superkhau · 2016-04-07T00:26:14Z

@jannyHou Could you create an issue for yourself to create a sample app for this? I believe you did implemented this feature right?

jannyHou · 2016-04-07T04:10:45Z

@superkhau yeah, thanks for reminding me. I create a story in TOB for it: https://github.com/strongloop-internal/scrum-loopback/issues/827

When a callback is omitted from a method on a model relation that supports promises, return that promise. This includes all the standard DAO methods, as well as any user-defined methods that return promises. e.g.: mylist.todos.create({name: 'Item 1'}) // returns Promise This API will use native ES6 promises if available. If not available, or to force the use of another Promise library, you must assign the global.Promise object. e.g.: global.Promise = require('bluebird') Relations affected: - BelongsTo - HasOne - HasMany - HasManyThrough - HasAndBelongsToMany - ReferencesMany - EmbedsOne Exceptions: The EmbedsMany relation has not been promisified, because most of the methods return synchronous values. The base relation getter method [e.g.: mylist.todos()] has not been promisified, due to its default caching behavior. New Methods: - getAsync(condition, cb) A new method "getAsync()" has been added to all relations except EmbedsMany, which always fetches from the datasource rather than from the cache. It takes an optional "where" condition (except for HasOne and BelongsTo) and an optional callback. If the callback is omitted, a Promise is returned.

altsang added the #community contribution label Feb 18, 2015

partap mentioned this pull request Feb 18, 2015

Promises Support #447

Closed

bajtos self-assigned this Feb 20, 2015

partap force-pushed the promises-related branch from 692c409 to ec5ec3c Compare February 21, 2015 21:36

bajtos mentioned this pull request Feb 24, 2015

relation-definition: remove trailing whitespace #469

Merged

bajtos reviewed Feb 24, 2015
View reviewed changes

coodoo mentioned this pull request Feb 24, 2015

Support promises strongloop/loopback#418

Closed

32 tasks

partap force-pushed the promises-related branch from ec5ec3c to 96bfac7 Compare February 25, 2015 02:06

partap force-pushed the promises-related branch 2 times, most recently from aaf8850 to f200f6b Compare March 2, 2015 23:31

partap force-pushed the promises-related branch from 9ac4d7d to df088f8 Compare March 30, 2015 22:24

bajtos reviewed Apr 1, 2015
View reviewed changes

partap added 2 commits April 3, 2015 12:40

Fix for getAsync(cb)

cc7f0d9

Test case for getAsync(cb)

39cabf3

bajtos pushed a commit that referenced this pull request Apr 7, 2015

Merge pull request #452 from partap/promises-related

2bdcce0

Promisify model relation methods

bajtos closed this Apr 7, 2015

bajtos removed the #community contribution label Apr 7, 2015

pulkitsinghal mentioned this pull request Apr 15, 2015

Use the promise API provided by loopback out-of-the-box ShoppinPal/StockUp#14

Closed

superkhau assigned crandmck and unassigned bajtos Apr 5, 2016

superkhau added the doc label Apr 5, 2016

bajtos mentioned this pull request Jul 10, 2017

add getAsync to documentation and clarify others #1127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Related Models Promisification #452

Related Models Promisification #452

partap commented Feb 18, 2015

slnode commented Feb 18, 2015

partap commented Feb 18, 2015

partap commented Feb 18, 2015

partap commented Feb 21, 2015

bajtos commented Feb 24, 2015

bajtos Feb 24, 2015

partap Feb 25, 2015

partap commented Feb 25, 2015

partap commented Feb 25, 2015

bajtos commented Mar 4, 2015

partap commented Mar 4, 2015

partap commented Mar 4, 2015

bajtos commented Mar 9, 2015

bajtos commented Mar 9, 2015

bajtos commented Mar 9, 2015

partap commented Mar 30, 2015

bajtos Apr 1, 2015

bajtos commented Apr 1, 2015

partap commented Apr 3, 2015

partap commented Apr 3, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

esco commented Apr 7, 2015

rmg commented Apr 7, 2015

michael-lumley commented Apr 5, 2016

superkhau commented Apr 5, 2016

crandmck commented Apr 7, 2016

superkhau commented Apr 7, 2016

jannyHou commented Apr 7, 2016

Related Models Promisification #452

Related Models Promisification #452

Conversation

partap commented Feb 18, 2015

slnode commented Feb 18, 2015

partap commented Feb 18, 2015

partap commented Feb 18, 2015

partap commented Feb 21, 2015

bajtos commented Feb 24, 2015

bajtos Feb 24, 2015

Choose a reason for hiding this comment

partap Feb 25, 2015

Choose a reason for hiding this comment

partap commented Feb 25, 2015

partap commented Feb 25, 2015

bajtos commented Mar 4, 2015

partap commented Mar 4, 2015

partap commented Mar 4, 2015

bajtos commented Mar 9, 2015

bajtos commented Mar 9, 2015

bajtos commented Mar 9, 2015

partap commented Mar 30, 2015

bajtos Apr 1, 2015

Choose a reason for hiding this comment

bajtos commented Apr 1, 2015

partap commented Apr 3, 2015

partap commented Apr 3, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

bajtos commented Apr 7, 2015

esco commented Apr 7, 2015

rmg commented Apr 7, 2015

michael-lumley commented Apr 5, 2016

superkhau commented Apr 5, 2016

crandmck commented Apr 7, 2016

superkhau commented Apr 7, 2016

jannyHou commented Apr 7, 2016