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

promisified version of req.logout #929

Open
anderskiaer opened this issue Aug 23, 2022 · 4 comments
Open

promisified version of req.logout #929

anderskiaer opened this issue Aug 23, 2022 · 4 comments

Comments

@anderskiaer
Copy link

Sorry if this has already been discussed (a quick search didn't give any hits).

Starting from 0.6.0, req.logout is an asynchronous function and requires a callback function. This is explained in the documentation https://www.passportjs.org/concepts/authentication/logout/:

app.post('/logout', function(req, res){
  req.logout(function(err) {
    if (err) { return next(err); }
    res.redirect('/');
  });
});

If possible, it would be nice to have a "promisified" version of req.logout available for writing more readable code. Another benefit: starting with express-5 (currently in beta), when functions returning a promise throw an error, express will automatically call next(err) (https://expressjs.com/en/guide/error-handling.html). Writing a passport based logout route could then probably be simplified to something like:

app.post('/logout', async function(req, res){
  await req.logout();
  res.redirect('/');
});

On a related note, in the current logout snippet example in the documentation, how is next made available to the callback function in cases of req.logout throwing an error? I would expected something like

-app.post('/logout', function(req, res){
+app.post('/logout', function(req, res, next){
-  req.logout(function(err) {
+  req.logout(err => {
     if (err) { return next(err); }
     res.redirect('/');
   });
 });

Thanks 🙇

jtojnar added a commit to jtojnar/wrcq that referenced this issue Sep 1, 2022
Using promisification since the logout function is async now:
jaredhanson/passport#929
@karlbecker
Copy link

Beyond that, it sure seems like

req#logout() is now an asynchronous function and requires a callback function as the last argument.

means passport should have a semantic versioning update to 1.0.0, since this change breaks existing calls to logout.

Agreed?

@anderskiaer
Copy link
Author

[...] passport should have a semantic versioning update to 1.0.0, since this change breaks existing calls to logout.

Agreed?

As long as passport is on 0.y.z, according to semver alone, the API can change on any release. Bumping major version when introducing breaking changes is a requirement when already at major version > 0.

@msrumon
Copy link

msrumon commented Jul 18, 2023

+1. It's long due to promisify this. So that we can use it like the following:

try {
  await req.logout();
  return res.redirect('/login');
} catch (error) {
  return next(error);
}

By the way, await promisify(req.logout)() didn't work for me for some reasons. Maybe I did something wrong, I'll investigate.

@tilleps
Copy link

tilleps commented Sep 7, 2023

@msrumon Example on how to promisify was posted on #536 (comment)

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

No branches or pull requests

4 participants