Support returning promises on Session prototype methods

[whatl3y]

Resolves issues #547, #607

This branch is similar to PRs #701 and #635 but with passing tests and test coverage, in addition to ensuring all methods return promises always, not only when there isn't a callback (I'm unsure why #635 only returns promises when there are no callbacks as returning a promise in this case doesn't harm anything, but let me know if I'm missing something).

This PR keeps the existing functionality with callbacks in tact, but now returns promises for the following prototype methods on Session.

1. Session.prototype.save
2. Session.prototype.reload
3. Session.prototype.destroy
4. Session.prototype.regenerate

This removes the ability to "chain" these methods together as was the original behavior, but not only was this chaining not being used anywhere in the repo, it doesn't really make sense to chain async methods together unless they're chains of returned promises. If I'm missing some historical context as to why this was the original behavior please correct me.

This also adds bluebird as a dependency for older versions of NodeJS that don't have promises in the standard library, but considering these older versions of NodeJS are either not supported anymore or won't be in the near future it might be worth removing as a dependency and just assuming Promise exists without falling back.

[jonchurch]

Hey there thanks for the contribution! I agree that promises are a pretty typical part of most modern libraries, and it would make sense to see them supported with session.

First, this would be a breaking change (no longer returning this) meaning promises, however they end up implemented, won't be added until a new semver major bump (along with whatever new improvements are slated for the next major). Nothing wrong with that, but wanted to make sure you knew any promise solution likely won't be merged right away.

I'm opposed to the approach of returning a Promise even when a callback is provided. You end up with unhandled Promise rejections if you're only using callbacks. Running the tests for this PR, you can see that several of the tests are giving warnings about unhandled promise rejections. I prefer the approach in #635 where you get either a Promise or a callback as resolution.

[jonchurch]

This is just some feedback, I'm not a maintainer of this Repo. But @ghinks and @gireeshpunathil are getting more directly involved in this project and I'd look to them to discuss what's coming next and what they'd want to see out of Promises for session.

[dougwilson]

If it helps @jonchurch I have the exact same general feedback you just posted. I was waiting for one of you folks to post before me :) The promise rejections that occur when users are using the callback API is of the biggest concern for sure.

[whatl3y]

Fair enough @jonchurch and @dougwilson, I made changes to only return promises if a callback is not provided to prevent unhandled rejections and reinstate returning this if a callback is provided to prevent the breaking change.

For any existing repos using callbacks their chaining would not break, but any new repos wanting to use promises with these methods can simply chain their promises together with .then()'s (or just subsequent awaits) as is the proper way to handle async methods in the first place.

[jonchurch]

@whatl3y Good work! I didn't even consider you could just return this for the callback based API and keep compatibility. I was wrong, this probably doesn't have to be a semver major bump.

[dougwilson]

So for some background @jonchurch , it unfortunately will as long as there are changes that expect a specific base class change to be in another store module. We found this out in the original Promises PR as an issue (I was working physically together with that author). For example, a store will need to inherit the Store class as a base class. Many stores will add express-session as a dependency to do this. And they will manage the version of express-session as well (ex https://github.com/mongodb-js/connect-mongodb-session/blob/master/package.json#L27).

This presents the situation of the current API: the calls that are made on req.session from within our index.js file need to expect that an older version of the base class will be in play, as many users will end up with the default ^ version requirement of npm resulting in a broken experience (including new users as well who would be installing the new versions). Typically if a change is to break existing express-session + store combos (like this one currently does) it should ideally be a major version of this module so the store modules can upgrade to support the new express-session semantics.

[dougwilson]

I didn't even consider you could just return this for the callback based API and keep compatibility. I was wrong, this probably doesn't have to be a semver major bump.

The current implementation, though it does this, is still not backwards compatible, though, as many of those methods do not require a callback, but would still have returned this; this implementation will now only return this if a callback was provided, breaking, for example, req.session.destroy().id (returning undefined in this PR vs the original ID in the current version).

[whatl3y]

Darn, this makes sense @dougwilson. I guess short of implementing a custom Promise class that contains properties of the calling session (and each subsequent promise created by .then, .catch, etc. etc. having these properties as well) I don't think there's anything more to do other than wait for the next major release to implement this.

Let me know if you guys think of anything creative or need help with anything else in the meantime.

[SimenB]

What about adding destroyAsync etc next to the versions that takes callbacks? Would mirror what e.g. bluebird does when promisifying.

(I couldn't see this mentioned in the linked issues/PRs, apologies if it has been).

For now I promisify manually when I need to:

const { promisify } = require('util');

async function destroySession(req) {
  const session = req.session;
  await promisify(session.destory.bind(session))();
}

[crawforde]

Any movement on this feature?

[mkvlrn-cm42]

Would very much like to have this implemented.