Promise API

[SimenB]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Currently SessionStore (and request.destroySession) are callback APIs. Ideally these should be Promise-based instead (or support both if you want to keep that).

Since adaptors (such as connect-redis) all expect callback APIs, that API probably has to be kept at least as backing implementation. So dual API (like Fastify itself) probably makes sense.

Motivation

It's more ergonomic, especially in larger async functions.

Example

Instead of

await promisify(sessionStore.set.bind(sessionStore))(
  session.sessionId,
  session,
);

//

await promisify(sessionStore.destroy.bind(sessionStore))(session.sessionId);

we could do

await sessionStore.set(session.sessionId, session);

//

await sessionStore.destroy(session.sessionId);

[zekth]

Let's keep both if possible. Would you work on this?

[SimenB]

I'm happy to work on this at some point in the future, but I cannot predict at what point I'll have time for it. So if somebody else is up for it that'd be great 🙂

[lokshunhung]

I took a look and here's 3 approaches to it.

1. Monkey patching methods on the Store instance after init (in ensureOptions())

   const sessionStore = options.store
   sessionStore.get = promisify(sessionStore.get.bind(sessionStore))
   sessionStore.set = promisify(sessionStore.set.bind(sessionStore))
   sessionStore.destroy = promisify(sessionStore.destroy.bind(sessionStore))

   It sort of works, but mutating the store instance is hacky, and might come with quirks when the store has other methods calling get, set, destroy and does not expect them to be promisified.

2. Create a new object with promisifed versions of get, set, destroy, then decorateRequest with a getter to this object here

   // Replaces: fastify.decorateRequest('sessionStore', { getter: () => options.store })
   const sessionStore = options.store
   const promisifyStore = {
     get: promisify(sessionStore.get.bind(sessionStore)),
     set: promisify(sessionStore.set.bind(sessionStore)),
     destroy: promisify(sessionStore.destroy.bind(sessionStore)),
   }
   fastify.decorateRequest('sessionStore', { getter: () => promisifyStore })

   However this loses reference to the original store. This also loses reference to other methods available on the store (Although it should not be part of the public API of request.sessionStore, it might still break integration with other things in unexpected way).

3. Wrap access to the store in a proxy object

   const sessionStore = options.store
   const promisifyStore = {
     get: promisify(sessionStore.get.bind(sessionStore)),
     set: promisify(sessionStore.set.bind(sessionStore)),
     destroy: promisify(sessionStore.destroy.bind(sessionStore)),
   }
   const proxyStore = new Proxy(options.store, {
     get(target, property) {
       if (property in promisifyStore) {
         return promisifyStore[property]
       }
       return target[property]
     }
   })
   fastify.decorateRequest('sessionStore', { getter: () => proxyStore })

   This approach does not monkey-patch any methods on the store instance, and allows methods of the original store to be accessed through request.sessionStore, while proxying get, set, destroy methods to the promisified functions.
   This means that the promisify methods are only available through accessing with request.sessionStore, so does not interfere with the store itself calling get, set, destroy in other methods.
   Proxy are available since Node v6 so compatibility is definitely not an issue.

---

I think option 3 is the way to go since it does not risk breaking integration with other things.

I'm happy to put together a PR for this :)

[mcollina]

I think the way to go is to support both callback and promises API, not one or the other.

[zekth]

we indeed need to support both way.

[lokshunhung]

Would this do the trick? The get set destroy inspect if a callback argument is passed, if not it would return a promise.

const promisifyStore = {
  get (sessionId, callback) {
    if (callback) {
      return store.get(sessionId, callback)
    }
    return new Promise((resolve, reject) => {
      store.get(sessionId, (err, session) => {
        if (err) reject(err)
        else resolve(session)
      })
    })
  },

  set (sessionId, session, callback) {
    if (callback) {
      return store.set(sessionId, session, callback)
    }
    return new Promise((resolve, reject) => {
      store.set(sessionId, session, (err) => {
        if (err) reject(err)
        else resolve()
      })
    })
  },

  destroy (sessionId, callback) {
    if (callback) {
      return store.destroy(sessionId, callback)
    }
    return new Promise((resolve, reject) => {
      store.destroy(sessionId, (err) => {
        if (err) reject(err)
        else resolve()
      })
    })
  }
}

const proxyStore = new Proxy(store, {
  get(target, property) {
    if (property in promisifyStore) {
      return promisifyStore[property]
    }
    return target[property]
  }
})

fastify.decorateRequest('sessionStore', { getter: () => proxyStore })

[lokshunhung]

I added some tests regarding the dual callback/promise support for this implementation.

I'll open a PR to gather feedbacks.

[mcollina]

I think it's actually the reverse, we want to be able to have a store return a promise and if so expect the callback to never be called.