epicweb-dev / cachified

🤑 wrap virtually everything that can store by key to act as cache with ttl/max-age, stale-while-validate, parallel fetch protection and type-safety support
MIT License
916 stars 26 forks source link

Adding a reporter changes return type of `getFreshValue` to `unknown` #70

Closed maxfi closed 8 months ago

maxfi commented 1 year ago

Without the reporter:

import { LRUCache } from "lru-cache";
import { cachified, CacheEntry } from "cachified";

const lru = new LRUCache<string, CacheEntry>({ max: 1000 });

function getUser() {
  return cachified({
    key: `user`,
    cache: lru,
    async getFreshValue() {
           //    ^? (property) CachifiedOptions<{ name: string; }>.getFreshValue: GetFreshValue<{ name: string }>
      return { name: 'Max' }
    },
  });
}

With the reporter:

import { LRUCache } from "lru-cache";
import { cachified, CacheEntry, verboseReporter } from "cachified";

const lru = new LRUCache<string, CacheEntry>({ max: 1000 });

function getUser() {
  return cachified({
    key: `user`,
    cache: lru,
    reporter: verboseReporter(),
    async getFreshValue() {
           //    ^? (property) CachifiedOptions<unknown>.getFreshValue: GetFreshValue<unknown>
      return { name: 'Max' }
    },
  });
}

Reproduction: Typescript playground

Is this possibly related to https://github.com/Xiphe/cachified/issues/5?

Xiphe commented 10 months ago

That's a really interesting edge-case! Thanks for reporting.

Upon debugging I found that the following works:

import { LRUCache } from "lru-cache";
import { cachified, CacheEntry, verboseReporter } from "cachified";

const lru = new LRUCache<string, CacheEntry>({ max: 1000 });

function getUser() {
  return cachified({
    key: `user`,
    cache: lru,
    reporter: verboseReporter(),
   getFreshValue: async () => {
      //          👆 use arrow function here 
      return { name: 'Max' }
    },
  });
}

But that doesn't seem like a sustainable solution to me :)

Another solution would be to explicitly type the call like this:

import { LRUCache } from "lru-cache";
import { cachified, CacheEntry, verboseReporter } from "cachified";

const lru = new LRUCache<string, CacheEntry>({ max: 1000 });

function getUser() {
  return cachified<{ name: string }>({
    //             👆 add type explicitly here
    key: `user`,
    cache: lru,
    reporter: verboseReporter(),
    async getFreshValue() {
      return { name: 'Max' }
    },
  });
}

But that's also something that I would expect TS to handle magically....


Unfortunately I don't have a simple non-breaking solution this this currently. I tend to think that it wouldn't hurt to make the reporter work on an un-inferred unknown similar to how the cache works but that would be a breaking change to the API.

Would be interested in different solutions to this 🙏

kentcdodds commented 8 months ago

I'm afraid I haven't taken the time to understand the situation here, but if anyone is interested in making a pull request to fix this we're going to be making a breaking change here soon so now would be a good time to have this ready to go.

Xiphe commented 8 months ago

Good point! Will have another look at this today or tomorrow.