passport
passport copied to clipboard
jaredhanson
Add support promises flow
Return promise from all api method for work with passport via await/yield/then/(without callback) flow.
It will be nice! All new modules uses async/await now via babel or nodejs 7. It really more simple and clean.
Since #225 and #243 have been created, it seems that a lot of people have been using promises and async/await for simpler control flow.
It's possible to promisify req.logIn in at least an Express app, but not obvious how to promisify passport.authenticate. It would be very helpful if promise support was added.
@jaredhanson What are your thoughts on introducing Promise support now that using Promises and async/await is more common?
@jaredhanson If I could reply to your sentiment about promises mentioned in your Stack Overflow answer from 2012:
Idiomatic Node.js
everyauth makes extensive use of promises, instead of Node's approach of using callbacks and closures. Promises are an alternative approach to async programming. While useful in some high-level situations, I wasn't comfortable with an authentication library forcing this choice upon my application.
Furthermore, I find that proper use of callbacks and closures yields concise, well architected (almost functional style) code. Much of the power of Node itself comes from this fact, and Passport follows suit.
I'd say that given the new async and await features of JavaScript, mentioned in the EMCAScript 2017 Draft (ECMA-262), and supported in stable Node.js releases with the --harmony-async-await command-line parameter, there is no reason to believe that callbacks yield "concise, well architected code" in relative terms. I believe the danger of creating "callback hell", where it's almost impossible to follow the control flow of the code, is the primary concern when writing well-architected code. Also, using Promises with async/await also doesn't prevent one from writing functional code.
Any decent, modern programming language with parallelism built in, and even old ones with parallelism tacked on, use Promises or Futures. Wikipedia has a list of languages that implement Promises, which includes the following languages with native Promise support: C++, Dart, Java, JavaScript, C#, Python, R, Scala, Swift, and Visual Basic, and more.
If you don't want to force Promises on your users, it would make sense to make a new major version of the Passport API while still fixing bugs in the old version.
Using a real concurrency construct like Promises should be the way forward for any Node.js library.
Just to note, Promises and callbacks aren't necessarily exclusive, you can support both in one function:
const asyncFunc = (callback) => {
const doAsyncThing = (cb) => {/* Do something asynchronous here, then call cb() */};
if (!callback) {
return new Promise(resolve => doAsyncThing(resolve));
} else {
doAsyncThing(callback);
}
}
Now both of these work, which means this would only be a feature release rather than a major release since it doesn't break anybody!
asyncFunc(() => { /* Callback when finished */ });
asyncFunc()
.then(() => { /* Resolve promise when finished */ });
@jaredhanson Do you have any thoughts on adding promise support to Passport?
Has any one tried to use the new promisify function builtin into the util module from node v 8.0 and up?
I tried using the promisify function to promisify things in Passport a few months ago. Although I can't remember much about what I did, I remember stopping at some point because I would have to change a lot of code within Passport and libraries it depended on. The library needs an overhaul, but @jaredhanson hasn't replied to a single message about this for almost 7 months now.
Someone should just fork the project and support promises.
What benefits would promises offer an application built on Express, using the middleware pattern?
For example, passport is used via middleware like:
app.post('/login', passport.authenticate('local'));
Why does that need to return a promise?
I am using express with graphql. So, I am using passport to secure my graphql endpoint but the issue is that graphql only works with promises and so it becomes necessary to wrap functions like passport.authenticate with another function which returns a promise
Also, most of the JS community (browser & node) is definitely moving fast towards adoption of async/await to handle async actions with promises over callback. So, adding support for promises would make passport easier to use with newer libraries which are coming out now and also in the future as they tend to mostly use promises (graphql being an example from the last few years)
@jaredhanson Also, re: middleware, Express 5 is going to have explicit promises support.
@mikemaccana Where did you read that? https://expressjs.com/en/guide/migrating-5.html no mention of promises... ?
@zenflow Yeah, because they are already supported (somehow) in 4.x: https://expressjs.com/en/advanced/best-practice-performance.html#use-promises
@michaelwalloschke Hmm, using promises is definitely advocated, but it doesn't look like there's built-in support...
Note that the second example under your link uses a wrap function on the request handler:
app.get('/', wrap(async (req, res, next) => {
...
@jaredhanson Mongoose and Sequelize prefer promises over callbacks now, so in a passport-local strategy, the common pattern will become
// strategy.js
...
User.findOne({ where: { email })
.then((existingUser) => { throw new myUserExistsError(`user with email ${email} already exists`; })
.then(() => User.create(userData))
.then(newUser => done(null, newUser, { message: 'success' })
.catch((error) => {
if (error instanceof myUserExistsError || error instanceof Sequelize.UniqueConstraintError) {
return done(error, false, { message: 'user already exists' } );
}
return done(error, false, { message: 'some other error' } );
});
...
// handler.js
(req, res, next) => {
return passport.authenticate('local-signup', (error, user, info) => {
if (error) return res.status(400).json({ message: info });
if (user) return res.status(200).json({ message: info });
return res.status(401).json({ message: info });
}
})(req, res, next);
where passport.authenticate can only accept a callback, making this a done hell when the strategy definition uses a few more thens and dones. Besides this forces many new to passport/promises into using an anti-pattern of mixing promises with callbacks, on top of ensuring done is called once and only once. Let's not assume people know how to convert between the two, especially people who follow the tutorials.
lol I think done() is dead. Promises have gotten popular due to await needing them, not because we like endless method chaining!
@zenflow See https://github.com/expressjs/express/pull/2237, the Express 5 release tracking bug list. Top item:
Add support for Promises in all handlers #2259
if you follow it, you'll end up at Router 2.0 which is https://github.com/pillarjs/router/pull/60
Await is way better than endless chaining and has been supported for a long time, it has my vote
Agreed. async/await has become the new de-facto standard, respected by many libraries such as Mongoose and Express.js
It would be only natural for Passport.js to support it as well, also given the growing demand
As of today passport's done() is the only oldschool error handling artifact in my app. It seems that everyone else moved to promises. You asked about benefits @jaredhanson and I think one obvious benefit is productivity and that if you teach a junior like myself it's easier to teach one concept of error handling, not a few different ones. Also mixing callbacks and promises makes testing tricky. Like others mentioned here this doesn't have to be a breaking change because promises and callbacks don't have to be exclusive- check out bcryptjs and mongoose for example. Thoughts?
I'm referring to LocalStrategy example where we pass errors to done(). In my app I have calls to database and async password hashing inside that callback which both return promises so if I only knew a way to make passport return a promise too I could get rid of done() and make it all look uniform. Does this make sense?
I thought about it all day- I've read entire documentation over and over again and it finally makes sense to me why you went with done() to cover all the possible outcomes as described in Verify Callback. As much as I wish there was a simpler way to do this I'm afraid that I can't come up with a more elegant solution that would work in Express 4.x. It looks like one may be possible in Express 5.x. Sorry for wasting your time!
What benefits would promises offer an application built on Express, using the middleware pattern?
I think one of the best arguments is how this can simply building unit tests. This article by Gustaf Holm shows how promises and async / await simply test code, and make it easier to write code that is easy to test.
Jared works at Auth0 now, and hasn't made any meaningful commits to this repo in forever. He does auth programming all day—for money. Why do you guys expect him to swoop in and save the day?
Passport doesn't seem like it's doing a ton of stuff. Maybe it's time for another hero to rise up and make some auth library from scratch?
Then again, it seems everyone could just use https://nodejs.org/api/util.html#util_util_promisify_original where needed.
