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

Promisify User model #1493

Merged
merged 1 commit into from
Aug 4, 2015
Merged

Conversation

PradnyaBaviskar
Copy link
Contributor

Connect #418

done();
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this test, User.login throws "Unhandled rejection Error: login failed". And so, when executing the test it says "Cannot call method 'then' of undefined". Whereas, https://github.com/strongloop/loopback/pull/1493/files#diff-5d81af95449574544e7317f3eda2badeR259, correctly returns fn.promise

@bajtos - can you help me?

@pulkitsinghal
Copy link

@PradnyaBaviskar It would seem that the loopback team intentionally limited the promisification to CRUD methods

@pulkitsinghal wrote: So the CRUD methods for models and relatedModels now use promises ... but something like User.login() still does not ... correct?

@bajtos wrote: Yes, that's a correct description of the current status.

But that doesn't mean we shouldn't get the other ones also promisified, thanks for opening this, +1

@@ -80,6 +80,9 @@ module.exports = function(User) {
cb = options;
options = undefined;
}

cb = cb || app.createPromiseCallback();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the app variable defined? Perhaps this is throwing an error that is later converted to the error message you are seeing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, are you relying on the fact that test/support.js is setting up a global app variable?

@bajtos bajtos self-assigned this Jul 2, 2015
@PradnyaBaviskar PradnyaBaviskar force-pushed the issue418-userModel branch 2 times, most recently from ebef568 to 6113eef Compare July 14, 2015 07:13
@PradnyaBaviskar
Copy link
Contributor Author

Done with promisifying User model.

@raymondfeng, can you please review?
/cc @bajtos

@bajtos
Copy link
Member

bajtos commented Aug 4, 2015

@slnode test please

bajtos added a commit that referenced this pull request Aug 4, 2015
@bajtos bajtos merged commit aa5c9e3 into strongloop:master Aug 4, 2015
@bajtos
Copy link
Member

bajtos commented Aug 4, 2015

Landed, thank you for the contribution!

@bajtos
Copy link
Member

bajtos commented Aug 4, 2015

FWIW, the Jenkins build is failing due to an issue that was fixed on master by 34eb0e1

@coodoo coodoo mentioned this pull request Aug 6, 2015
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants