Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Re-try #2135

Merged
merged 2 commits into from
Dec 4, 2017
Merged

Re-try #2135

merged 2 commits into from
Dec 4, 2017

Conversation

brianlovin
Copy link
Contributor

@brianlovin brianlovin commented Dec 4, 2017

Needs more testing...

@brianlovin
Copy link
Contributor Author

@mxstbr just pushed some changes ready for review (ignore some eslint cleanup like removing unused model functions)...

One problem we had was that we weren't returning from a promise which might have been throwing errors. Also: in our current authentication implementation we are mixing callbacks and promises, warned against by eslint and more info here: https://spin.atomicobject.com/2017/04/06/nodejs-promises-callbacks/ - does that look like something we should fix now?

})
.catch(err => {
done(err);
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I've fixed this on the #2096 branch too, but I did

return getUser({ id })
  .then(user => {
    done(null, user);
    return user;
  })

Unsure if that makes any difference tho.

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Sure

@mxstbr
Copy link
Contributor

mxstbr commented Dec 4, 2017

Also: in our current authentication implementation we are mixing callbacks and promises, warned against by eslint

Lollll

I wish Passport would have a Promise interface

Alas, it doesn't, so no way around that. jaredhanson/passport#536

@brianlovin brianlovin merged commit 3de0e8b into master Dec 4, 2017
@brianlovin brianlovin deleted the reject-iris branch December 4, 2017 18:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants