fix: delayed background de-duplication and deprecate deprecate-staleRefreshTimeout (#134)

* fix: de-duplicate delayed background refreshes

prior to this syncrounous calls or cases with staleRefreshTimeout > 0 would
queue up a lot of work and thereby behave different to the default de-duplication
behavior

this change queues up background refreshes imediately and then delays the
potentially bigger work in `getFreshValue`

fix #132

* fix: deprecate staleRefreshTimeout

ref #132

* refactor: remove unnecessary promise wrapper

Author: Xiphe

README.md

@@ -224,15 +224,6 @@ interface CachifiedOptions<Value> {
    * Default: `Infinity`
    */
   fallbackToCache?: boolean | number;
-  /**
-   * Amount of time in milliseconds before revalidation of a stale
-   * cache entry is started
-   *
-   * Must be positive and finite
-   *
-   * Default: `0`
-   */
-  staleRefreshTimeout?: number;
   /**
    * Promises passed to `waitUntil` represent background tasks which must be
    * completed before the server can shutdown. e.g. swr cache revalidation

src/cachified.spec.ts

@@ -590,6 +590,45 @@ describe('cachified', () => {
     expect(cache.get('weather')?.value).toBe('🌈');
   });
 
+  it('de-duplicates stale refreshes', async () => {
+    const cache = new Map<string, CacheEntry>();
+    const log: string[] = [];
+    const backgroundTasks: Promise<unknown>[] = [];
+
+    const getValue = (id: string) => {
+      return cachified({
+        cache,
+        waitUntil(p) {
+          backgroundTasks.push(p);
+        },
+        key: `test`,
+        ttl: 10,
+        swr: 50,
+        getFreshValue() {
+          log.push(id);
+          return id;
+        },
+      });
+    };
+
+    // Warm up cache
+    await getValue('1');
+
+    // These calls are background refreshing. Second one is de-duplicated.
+    currentTime = 15;
+    const call2 = getValue('2');
+    const call3 = getValue('3');
+
+    // They resolve with stale value
+    expect(await call3).toBe('1');
+    expect(await call2).toBe('1');
+
+    await Promise.all(backgroundTasks);
+
+    // Only one refresh was executed
+    expect(log).toEqual(['1', '2']);
+  });
+
   it('gets different values for different keys', async () => {
     const cache = new Map<string, CacheEntry>();
 

src/common.ts

@@ -184,6 +184,8 @@ export interface CachifiedOptions<Value> {
    * Must be positive and finite
    *
    * Default: `0`
+   * @deprecated manually delay background refreshes in getFreshValue instead
+   * @see https://github.com/epicweb-dev/cachified/issues/132
    */
   staleRefreshTimeout?: number;
   /**

src/getCachedValue.ts

@@ -54,24 +54,28 @@ export async function getCachedValue<Value>(
     if (staleRefresh) {
       // refresh cache in background so future requests are faster
       context.waitUntil(
-        Promise.resolve().then(async () => {
-          await sleep(staleRefreshTimeout);
-          report({ name: 'refreshValueStart' });
-          await cachified({
-            ...context,
-            getFreshValue({ metadata }) {
-              return context.getFreshValue({ metadata, background: true });
-            },
-            forceFresh: true,
-            fallbackToCache: false,
+        cachified({
+          ...context,
+          async getFreshValue({ metadata }) {
+            /* TODO: When staleRefreshTimeout option is removed we should
+               also remove this or set it to ~0-200ms depending on ttl values.
+               The intention of the delay is to not take sync resources for
+               background refreshing – still we need to queue the refresh
+               directly so that the de-duplication works.
+               See https://github.com/epicweb-dev/cachified/issues/132 */
+            await sleep(staleRefreshTimeout);
+            report({ name: 'refreshValueStart' });
+            return context.getFreshValue({ metadata, background: true });
+          },
+          forceFresh: true,
+          fallbackToCache: false,
+        })
+          .then((value) => {
+            report({ name: 'refreshValueSuccess', value });
           })
-            .then((value) => {
-              report({ name: 'refreshValueSuccess', value });
-            })
-            .catch((error) => {
-              report({ name: 'refreshValueError', error });
-            });
-        }),
+          .catch((error) => {
+            report({ name: 'refreshValueError', error });
+          }),
       );
     }