jaredwray / cacheable

Caching for Nodej.js
https://cacheable.org
MIT License
1.55k stars 163 forks source link

Date objects are stored as strings #843

Closed lugrinder closed 1 week ago

lugrinder commented 1 week ago

Describe the bug Date properties are stored as string instead of object. On many ORM, date/time fields are returned as Date objects, so cache its results causes this issue. I think that versions before keyv works, but not really sure, I've detected the issue after updating to v6. If this will be the default behavior, please fix the documentation and add typescript type guard to ensure that only valid values can be cached, or better, transform the result type for wrap for all Date properties to be the original returned value (Date) and string: Date | string.

How To Reproduce (best to provide workable code or tests!) Cache any value with a Date property.

lugrinder commented 1 week ago

I can confirm that v5 stores Date as object and don't have this issue. I've fixed it, adding the next typescript type to the returned value of wrap that have Date properties, so typescript warns about this if I've tried to use as Date:

type StringifyObject<T> = {
    [P in keyof T]: Extract<T[P], object> extends never ? T[P] : T[P] | string
}

At least, add something similar to the returned wrap value.

jaredwray commented 1 week ago

@lugrinder - thanks for letting us know. Can you tell me what package you are seeing this on? Is this cache-manager?

Also, can you give me an example of where you are using this code to fix it?

lugrinder commented 1 week ago

Yes, cache-manager. You can see here a running example.

jaredwray commented 1 week ago

got it. I see. Where are you adding this code to fix it?

type StringifyObject<T> = {
    [P in keyof T]: Extract<T[P], object> extends never ? T[P] : T[P] | string
}
lugrinder commented 1 week ago

I'm not modified the original code, I've done something like this:

async function getData(name: string) {
    const result = await cache.wrap(name, async () => {
        return new Promise((resolve) => {
            resolve({
                a: 'hello',
                b: new Date
            })
        })
    })
    return result as StringifyObject<typeof result>
}

If you do it inside wrap, there is no need to add in all calls to wrap, you can infer from the wrapped promise. The question is, if not possible to store objects as v5, at least make a union for the transformed types and ensure that typescript will warn if you don't take care.

jaredwray commented 1 week ago

@lugrinder - thanks for the info and I am going to see what we can do to help with this. Also, if you are doing just an in-memory you could probably just use CacheableMemory from the cacheable library as it is now and has a wrap() functionality with no parsing. If you are using a persistent store then no need to try it out.

jaredwray commented 1 week ago

@lugrinder - just another thought is that you can set Keyv with another parser that handles Date better such as superjson. Here is an example of how to do that:

import Keyv from 'keyv';
import superjson from 'superjson'
import {createCache} from 'cache-manager';

const keyv = new Keyv({ serialize: superjson.stringify, deserialize: superjson.parse });
const cache = createCache({ stores: [keyv] });
jaredwray commented 1 week ago

@lugrinder - looks like the best way is to do the parsing option. Will write more on this a bit later but going to close this. Let me know if you need any other help.