Caching arrays

[juckerf]

First: thank you for initiating this project. I really like it :-)

I have one problem: if the data processed by the hooks is an array instead of an object, the hooks don't work. When reading the cached array feathers fails with the following exception: ` var duration = (0, _moment2.default)(hook.result.cache.expiresOn).format('DD MMMM YYYY - HH:mm:ss'); ^

TypeError: Cannot read property 'expiresOn' of undefined at Command.callback (C:\Users\fabi\dev\ba\backend\node_modules\feathers-hooks-rediscache\lib\library.js:442:65) at normal_reply (C:\Users\fabi\dev\ba\backend\node_modules\redis\index.js:726:21) at RedisClient.return_reply (C:\Users\fabi\dev\ba\backend\node_modules\redis\index.js:824:9) at JavascriptRedisParser.returnReply (C:\Users\fabi\dev\ba\backend\node_modules\redis\index.js:192:18) at JavascriptRedisParser.execute (C:\Users\fabi\dev\ba\backend\node_modules\redis-parser\lib\parser.js:553:10) at Socket. (C:\Users\fabi\dev\ba\backend\node_modules\redis\index.js:274:27) at emitOne (events.js:96:13) at Socket.emit (events.js:191:7) at readableAddChunk (_stream_readable.js:178:18) at Socket.Readable.push (_stream_readable.js:136:10) `

I guess the reason for this exception is, that in the after hooks (when saving the array to redis) hook.result.cache is created on hook result (which should be an object, but is an array in my case) (https://github.com/idealley/feathers-hooks-rediscache/blob/master/src/hooks/cache.js#L13) and some cache properties are assigned to hook.result.cache (https://github.com/idealley/feathers-hooks-rediscache/blob/master/src/hooks/redis.js#L67). And since hook.result is an array instead of an object, the cache property don't really gets added (because arrays can't have properties) and is not saved in redis.

When trying to read the data from redis in a subsequent request this leads to the error listed above.

I'd be really happy to help you to fix this issue, but I'd like to know what your thoughts are about handling arrays.

Some different ways that came to my mind:

• don't handle arrays at all => "force" users of the hooks to encapsulate arrays in objects (e.g. {"data": ["obj1", "obj2", ...]}) before they get processed by the hooks
• if processed data is an array: encapsulate it in an object before saving to redis and extract it when loading from redis (follow up questions: how to handle the cache property in the http response)
• ommit the cache property and directly check if the data is already cached or not (assumes the cache property is not really needed in the http response)

[idealley]

Thank you for your help on this. Actually I developed these hooks for caching data coming out of a "headless CMS", I parse the data (markdown, date according to corporate guidelines etc.) and I always have objects... such as:

{
    items: [],
    category: {},
    _sys: {},
    // and now:
    cache: {}
}

I think it is a bad idea to force people to do something ;) and I think it is best if hook is just "plug and play" thus I like your encapsulating idea.

As far as the my data goes to different systems (Website, apps, etc) I do not mind the overhead of having the cache object. With your additional hook to remove it become flexible as we can use conditional hooks, based on the transport layer, the request or whatever to apply the hookRemoveCacheInformation or not, on some routes or on all etc. I find this very flexible.

We could simply use a check such as Array.isArray() and if true wrap it in an object:

{
    wrapped: [],
    cache: {}
}

We could assume that if the person uses arrays in responses, that person just wants its data thus discard all together the cache property in the before hook?

In the before hook we would then need to check if the wrapped (or a better name) property exists if so, we return the array only, if not the whole object.

Thus we would have:

1. Use arrays and your data is cached and untouched
2. Use objects and you can have:
   1. a cache object added to your object
   2. just your data with the use of the hookRemoveCacheInformation

One thing that is missing though is setting the cache expiration in the http header. That could be just the information needed by array users?

What do you think?

[juckerf]

I really like your proposal to solve this problem. I agree with your assumption, that people who use arrays don't want/need the cache property at all (otherwise they could switch to an object containing the array). The cache expiration http header would indeed be a nice way for array users to know about the cache lifetime. Alltogether I think this solution would be very transparent and "plug'n'play" for the users.

If I find the time, I'll try to implement this and make a PR

[idealley]

Can you check if this is ok with you? I have added in the branch "manage-arrays" a solution for this issue

[juckerf]

looks perfectly fine to me. thanks for this implementation! the only "downside" is that 'wrapped' is now like a reserved keyword, but I think that's the tradeoff.

[idealley]

True... I did not think of that.

[idealley]

I guess that could be configurable. I also thought to add the log part as an option

[juckerf]

what about putting the wrapped array into the cache property? like

    if (Array.isArray(hook.result)) {
        const array = hook.result;

        hook.result = {};
        hook.result.cache = {
          wrapped: array
          cached: false,
          duration: options.duration || 3600 * 24
          ...

and then check for hook.result.cache.wrapped

(sidenote: with cache the module already has kind of a reserved keyword^^)

or like you said: make the name of wrapped configurable

[idealley]

I like that! And if somebody complains about the cache reserved keyword then, we can make it configurable. Perfect. I will implement it.