server.method doesn't resolve promise with cache options

[aegyed91]

You register a server method and that method returns a promise and specify caching options as the 3rd argument. The thing dies.

server.method('test', function () {
  return Promise.resolve('works');
}, {
  // if you remove the cache options it works as expected
  cache: {
    cache: 'redisCache',
    expiresIn: 30 * 1000,
    generateTimeout: 100
  }
});

If you omit the cache options everything runs great.

Here is a working gist how to reproduce.

Tested on node v6.1.0, v4.3.0.

stack trace:

Debug: internal, implementation, error
    TypeError: Uncaught error: Cannot read property 'then' of undefined
    at server.route.handler (/Users/tsm/Sites/__projects/hapi-api/reproduce.js:36:7)
    at Object.exports.execute.internals.prerequisites.internals.handler.callback [as handler] (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/lib/handler.js:96:36)
    at /Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/lib/handler.js:30:23
    at internals.Protect.run.finish [as run] (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/lib/protect.js:64:5)
    at exports.execute.finalize (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/lib/handler.js:24:22)
    at each (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/lib/request.js:383:16)
    at iterate (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/node_modules/items/lib/index.js:36:13)
    at done (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/node_modules/items/lib/index.js:28:25)
    at internals.Auth.test.internals.Auth._authenticate (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/lib/auth.js:210:16)
    at internals.Auth.test.internals.Auth.authenticate (/Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/lib/auth.js:202:17)
Debug: internal, implementation, error
    TypeError: Cannot read property 'apply' of undefined
    at /Users/tsm/Sites/__projects/hapi-api/node_modules/hapi/node_modules/catbox/node_modules/hoek/lib/index.js:850:21
    at nextTickCallbackWith0Args (node.js:453:9)
    at process._tickDomainCallback (node.js:423:13)

[matthieusieben]

Same issue here.

This is due to the fact that the provided function is wrapped inside a server cache (when options.cache is defined), but the function exposed (through server.method) is not promisifyed (or "denormalized") when options.callback === false (See the differences between the two this._assign calls in methods.js here and here).

This won't be an easy fix as it would break compatibility with previous versions. But having the same behavior with and without cache is the best option IMHO.

[matthieusieben]

@tsm91 You can actually still use your method function, but not as a promise. Note that you need to specify callback: false to use the return value of your function as result for your server method (and for the result to be cached).

Here is an example with caching:

server.method('test', function (arg) {
  return Promise.resolve('Hello ' + arg + '!');
}, {
  callback: false,
  cache: {
    // cache options
  }
});
// The method was "normalized" because of the caching
server.method.test('World', function(err, result) {
  // result === 'Hello World!'
});

Without caching:

server.method('test', function (arg) {
  return Promise.resolve('Hello ' + arg + '!');
}, {
  callback: false,
  cache: false
});
// The method is exposed "as is" (unless bound)
server.method.test('World').then(function (result) {
  // result === 'Hello World!'
});

[aegyed91]

yeah, that is a boomer. i dont like to mix promises with callbacks

[matthieusieben]

As a workaround, you can use a proxy that re-creates a promise from a normalized function:

server.method('testCached', function (arg1, arg2) {
    return Promise.resolve(arg1 + arg2);
}, {
    cache: true,
    callback: false,
});
// At this point server.methods.testCached has been normalized
server.method('test', promisify(server.methods.testCached), {
    callback: false,
});

function promisify(fn) {
    return function () {
        var self = this;
        var args = new Array(arguments.length + 1);
        for (var i = arguments.length - 1; i >= 0; i--) {
            args[i] = arguments[i];
        }
        return new Promise(function (resolve, reject) {
            args[args.length - 1] = function(err, result) {
                if (err) {
                    reject(err);
                } else {
                    resolve(result);
                }
            };
            fn.apply(self, args);
        });
    };
}

You will need to always provide the exact number of arguments (as it is generally the case with server methods). There may be more adequate ways of promisifying (Bluebird) FYI @rmedaer

[nlf]

this feels like a bug, it should be consistent whether caching is enabled or not

[devinivy]

The problem is that caching requires callback-style over promises because the callback takes a third argument for TTL. I guess in the case that a promise is returned, that argument could just be omitted. But then the promise vs. callback API is asymmetrical, which is a just a different kind of API wart.

[nlf]

or we could make a breaking change and resolve the promise with an object, or an array (value is the first item, ttl the second), or expose it some other way. there's a lot we can do to make this work in a more consistent manner.

[aegyed91]

@matthieusieben in the end i do not use the server.method approach but something like this:

some util file excerpt:

let todoListCache;

export function listTodosCache(req) {
  if (!todoListCache) {
    todoListCache = req.server.cache({
      expiresIn: 60 * 1000,
      generateTimeout: 100,
      segment: '!todoList',
      generateFunc({ user, pageNum, i18n }, next) {
        return listTodos(user, pageNum, i18n)
          .then(todoList => next(null, todoList))
          .catch(err => next(err));
      }
    });

    Promise.promisifyAll(todoListCache);
  }

  return todoListCache;
}

controller file excerpt:

handler(req, res) {
    listTodosCache(req)
      .getAsync({
        id: `${req.user.id}-${req.query.pageNum}`,
        user: req.user, pageNum:
        req.query.pageNum,
        i18n: req.i18n
      })
      .then(todos => res(todos))
      .catch(err => res(err));
  },

[steevhise]

This should at least be mentioned in the docs.

[TangMonk]

+1, Now I am using server method for maintain Redis and MQTT connection, I cannot using cache with promise together, It make me annoying

[einnjo]

With the increased acceptance of promises and async/await in nodejs 8 and https://github.com/hapijs/hapi/pull/3486 landing recently, I believe the cache api does feel inconsistent.

I like the idea that @nlf proposed. Resolving a cached thenable server method with an object/array that contains the the result and its ttl would be more natural. Promise in, Promise out vs Promise in, Cb out.

[lock[bot]]

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.