Passport: Migrating to ES6

Created on 18 Sep 2017 · 5Comments · Source: jaredhanson/passport

I would love migrating the code and the README from ES5 to ES6, shall I go ahead ?

Source

itaditya

👎1

Most helpful comment

For a comparison take an example from the README

Original Code

passport.use(new LocalStrategy(
  function(username, password, done) {
    User.findOne({ username: username }, function (err, user) {
      if (err) { return done(err); }
      if (!user) { return done(null, false); }
      if (!user.verifyPassword(password)) { return done(null, false); }
      return done(null, user);
    });
  }
));

ES6 Code

passport.use(new LocalStrategy(async (username, password, done) => {
  try {
    const user = await User.findOne({ username });
    if (!user) { return done(null, false); }
    if (!user.verifyPassword(password)) { return done(null, false); }
    return done(null, user);
  } catch (err) { return done(err); }
}));

itaditya on 18 Sep 2017

👍4 👎1

All 5 comments

For a comparison take an example from the README

Original Code

passport.use(new LocalStrategy(
  function(username, password, done) {
    User.findOne({ username: username }, function (err, user) {
      if (err) { return done(err); }
      if (!user) { return done(null, false); }
      if (!user.verifyPassword(password)) { return done(null, false); }
      return done(null, user);
    });
  }
));

ES6 Code

passport.use(new LocalStrategy(async (username, password, done) => {
  try {
    const user = await User.findOne({ username });
    if (!user) { return done(null, false); }
    if (!user.verifyPassword(password)) { return done(null, false); }
    return done(null, user);
  } catch (err) { return done(err); }
}));

itaditya on 18 Sep 2017

👍4 👎1

I wonder if calling "done" in the "try" block can have side effects, e.g. if an exception is thrown in the "done" function, and done is called again?

nicokaiser on 10 Oct 2017

Would you still need the "done" if it's es6? Sorry, I'm a noob.

Jadex1 on 12 Oct 2017

@itaditya Migrating would be a breaking change which would affect the ability to use passport on older Node versions.

Next, your example rewrite saves 1 linebreak, adding little of value.

Finally, the ECMAScript version in which passport is written is an implementation detail. Barring things like import (which, in latest Node, is not an issue due to import <- require interop), users don't need to know if passport is written in ES6 or ES5. The fact that the package is written in ES5 makes it more portable, full stop. If converted to ES6+, adding a build step to support older environments is (a) unnecessary and (b) adds to package complexity. To my knowledge, the maintainer of this package has never encountered any maintenance issues related to use of ES5; however, by migrating the code base to ES6, the maintenance burden would increase. Breaking compatibility with older Node versions is not an option.

In short, don't advocate for this kind of change for the sake of doing so. If you are eager to contribute, I suggest working on more substantive issues.