Fix handling of "undefined"

README.md

@@ -94,7 +94,9 @@ Loaders and caches are composed out of the following building blocks.
 3. **Data Source** - primary source of truth of data, that can be used for populating caches. Used in a strictly read-only mode.
 
 - `layered-loader` will try loading the data from the data source defined for the Loader, in the following order: InMemory, AsyncCache, DataSources. In case `undefined` value is the result of retrieval, next source in sequence will be used, until there is either a value, or there are no more sources available;
-- `null` is considered to be a value, and if the data source returns it, subsequent data source will not be queried for data;
+- `null` and `undefined` have different semantics:
+  - `null` means "value was successfully resolved, but it is empty" - this **will be cached** and subsequent data sources will not be queried;
+  - `undefined` means "value was not resolved" - this **will NOT be cached** and the next data source in the sequence will be queried. If all data sources return `undefined`, the Loader returns `undefined` without caching anything;
 - If non-last data source throws an error, it is handled using configured ErrorHandler. If the last data source throws an error, and there are no remaining fallback data sources, an error will be thrown by the Loader.
 - If any caches (InMemoryCache or AsyncCache) precede the source, that returned a value, all of them will be updated with that value;
 - If there is an ongoing retrieval operation for the given key, promise for that retrieval will be reused and returned as a result of `loader.get`, instead of starting a new retrieval.

lib/GroupLoader.ts

@@ -68,16 +68,7 @@ export class GroupLoader<LoadedValue, LoadParams = string, LoadManyParams = Load
         return cachedValue
       }
 
-      return this.loadFromLoaders(key, group, loadParams).then((finalValue) => {
-        if (finalValue !== undefined) {
-          return finalValue
-        }
-
-        if (this.throwIfUnresolved) {
-          throw new Error(`Failed to resolve value for key "${key}", group "${group}"`)
-        }
-        return undefined
-      })
+      return this.loadFromLoaders(key, group, loadParams)
     })
   }
 
@@ -130,21 +121,22 @@ export class GroupLoader<LoadedValue, LoadParams = string, LoadManyParams = Load
           throw err
         }
       })
-      if (resolvedValue !== undefined || index === this.dataSources.length - 1) {
-        if (resolvedValue === undefined && this.throwIfUnresolved) {
-          throw new Error(`Failed to resolve value for key "${key}", group "${group}"`)
-        }
-
-        const finalValue = resolvedValue ?? null
+      // null means "resolved to empty value" and should be cached
+      // undefined means "not resolved" and should not be cached
+      if (resolvedValue !== undefined) {
         if (this.asyncCache) {
-          await this.asyncCache.setForGroup(key, finalValue, group).catch((err) => {
+          await this.asyncCache.setForGroup(key, resolvedValue, group).catch((err) => {
             this.cacheUpdateErrorHandler(err, key, this.asyncCache!, this.logger)
           })
         }
-        return finalValue
+        return resolvedValue
       }
     }
 
+    // All data sources returned undefined - value not resolved
+    if (this.throwIfUnresolved) {
+      throw new Error(`Failed to resolve value for key "${key}", group "${group}"`)
+    }
     return undefined
   }
 

lib/Loader.ts

@@ -143,16 +143,7 @@ export class Loader<LoadedValue, LoadParams = string, LoadManyParams = LoadParam
       }
 
       // No cached value, we have to load instead
-      return this.loadFromLoaders(key, loadParams).then((finalValue) => {
-        if (finalValue !== undefined) {
-          return finalValue
-        }
-
-        if (this.throwIfUnresolved) {
-          throw new Error(`Failed to resolve value for key "${key}"`)
-        }
-        return undefined
-      })
+      return this.loadFromLoaders(key, loadParams)
     })
   }
 
@@ -164,21 +155,22 @@ export class Loader<LoadedValue, LoadParams = string, LoadManyParams = LoadParam
           throw err
         }
       })
-      if (resolvedValue !== undefined || index === this.dataSources.length - 1) {
-        if (resolvedValue === undefined && this.throwIfUnresolved) {
-          throw new Error(`Failed to resolve value for key "${key}"`)
-        }
-
-        const finalValue = resolvedValue ?? null
+      // null means "resolved to empty value" and should be cached
+      // undefined means "not resolved" and should not be cached
+      if (resolvedValue !== undefined) {
         if (this.asyncCache) {
-          await this.asyncCache.set(key, finalValue).catch((err) => {
+          await this.asyncCache.set(key, resolvedValue).catch((err) => {
             this.cacheUpdateErrorHandler(err, key, this.asyncCache!, this.logger)
           })
         }
-        return finalValue
+        return resolvedValue
       }
     }
 
+    // All data sources returned undefined - value not resolved
+    if (this.throwIfUnresolved) {
+      throw new Error(`Failed to resolve value for key "${key}"`)
+    }
     return undefined
   }
 

lib/types/DataSources.ts

@@ -53,6 +53,15 @@ export interface GroupCache<LoadedValue> extends GroupWriteCache<LoadedValue> {
   getExpirationTimeFromGroup: (key: string, group: string) => Promise<number | undefined>
 }
 
+/**
+ * Data source interface for retrieving values.
+ *
+ * Return value semantics:
+ * - Return the actual value when found
+ * - Return `null` to indicate "value was resolved but is empty" - this WILL be cached
+ * - Return `undefined` to indicate "value was not resolved" - this will NOT be cached,
+ *   and the next data source in the sequence will be queried
+ */
 export interface DataSource<LoadedValue, LoadParams = string, LoadManyParams = LoadParams extends string ? undefined : LoadParams> {
   get: (loadParams: LoadParams) => Promise<LoadedValue | undefined | null>
 
@@ -62,6 +71,15 @@ export interface DataSource<LoadedValue, LoadParams = string, LoadManyParams = L
   name: string
 }
 
+/**
+ * Group data source interface for retrieving values within groups.
+ *
+ * Return value semantics:
+ * - Return the actual value when found
+ * - Return `null` to indicate "value was resolved but is empty" - this WILL be cached
+ * - Return `undefined` to indicate "value was not resolved" - this will NOT be cached,
+ *   and the next data source in the sequence will be queried
+ */
 export interface GroupDataSource<LoadedValue, LoadParams = string, LoadManyParams = LoadParams extends string ? undefined : LoadParams> {
   getFromGroup: (loadParams: LoadParams, group: string) => Promise<LoadedValue | undefined | null>
   getManyFromGroup: (keys: string[], group: string, loadParams?: LoadManyParams) => Promise<LoadedValue[]>

package.json

@@ -50,7 +50,7 @@
   ],
   "homepage": "https://github.com/kibertoad/layered-loader",
   "dependencies": {
-    "ioredis": "^5.6.1",
+    "ioredis": "^5.8.2",
     "toad-cache": "^3.7.0"
   },
   "devDependencies": {
@@ -59,8 +59,8 @@
     "@vitest/coverage-v8": "^3.2.0",
     "del-cli": "^7.0.0",
     "rfdc": "^1.4.1",
-    "vitest": "^3.2.0",
-    "typescript": "^5.8.3"
+    "vitest": "^3.2.4",
+    "typescript": "^5.9.3"
   },
   "files": ["README.md", "LICENSE", "dist/*"]
 }

test/GroupLoader-main.spec.ts

@@ -381,6 +381,42 @@ describe('GroupLoader Main', () => {
       expect(value).toEqual(user1)
     })
 
+    it('does not update cache when all data sources return undefined', async () => {
+      const cache = new DummyGroupedCache(userValuesUndefined)
+      const setSpy = vitest.spyOn(cache, 'setForGroup')
+
+      const operation = new GroupLoader({
+        asyncCache: cache,
+        dataSources: [new DummyGroupedLoader(userValuesUndefined)],
+      })
+
+      const value = await operation.get(user1.userId, user1.companyId)
+
+      expect(value).toBeUndefined()
+      expect(setSpy).not.toHaveBeenCalled()
+    })
+
+    it('updates cache when data source returns null (explicit empty value)', async () => {
+      const cache = new DummyGroupedCache(userValuesUndefined)
+      const setSpy = vitest.spyOn(cache, 'setForGroup')
+
+      const userValuesNull = {
+        [user1.companyId]: {
+          [user1.userId]: null,
+        },
+      }
+
+      const operation = new GroupLoader({
+        asyncCache: cache,
+        dataSources: [new DummyGroupedLoader(userValuesNull as unknown as typeof userValues)],
+      })
+
+      const value = await operation.get(user1.userId, user1.companyId)
+
+      expect(value).toBeNull()
+      expect(setSpy).toHaveBeenCalledWith(user1.userId, null, user1.companyId)
+    })
+
     it('logs error during load', async () => {
       const consoleSpy = vitest.spyOn(console, 'error')
       const operation = new GroupLoader({
@@ -399,11 +435,12 @@ describe('GroupLoader Main', () => {
       const operation = new GroupLoader({ dataSources: [loader] })
 
       const value = await operation.get(user1.userId, user1.companyId)
-      expect(value).toBeNull()
+      expect(value).toBeUndefined()
 
+      // groupValues = null causes optional chaining to return undefined
       loader.groupValues = null
       const value2 = await operation.get(user1.userId, user1.companyId)
-      expect(value2).toBeNull()
+      expect(value2).toBeUndefined()
 
       loader.groupValues = userValues
       const value3 = await operation.get(user1.userId, user1.companyId)

test/Loader-main.spec.ts

@@ -431,6 +431,36 @@ describe('Loader Main', () => {
       expect(value).toBe('value')
     })
 
+    it('does not update cache when all data sources return undefined', async () => {
+      const cache = new DummyCache(undefined)
+      const setSpy = vitest.spyOn(cache, 'set')
+
+      const operation = new Loader({
+        asyncCache: cache,
+        dataSources: [new DummyDataSource(undefined)],
+      })
+
+      const value = await operation.get('key')
+
+      expect(value).toBeUndefined()
+      expect(setSpy).not.toHaveBeenCalled()
+    })
+
+    it('updates cache when data source returns null (explicit empty value)', async () => {
+      const cache = new DummyCache(undefined)
+      const setSpy = vitest.spyOn(cache, 'set')
+
+      const operation = new Loader({
+        asyncCache: cache,
+        dataSources: [new DummyDataSource(null as unknown as undefined)],
+      })
+
+      const value = await operation.get('key')
+
+      expect(value).toBeNull()
+      expect(setSpy).toHaveBeenCalledWith('key', null)
+    })
+
     it('logs error during load', async () => {
       const consoleSpy = vitest.spyOn(console, 'error')
       const operation = new Loader({ dataSources: [new ThrowingLoader()], throwIfLoadError: true })
@@ -446,7 +476,7 @@ describe('Loader Main', () => {
       const operation = new Loader({ dataSources: [loader] })
 
       const value = await operation.get('value')
-      expect(value).toBeNull()
+      expect(value).toBeUndefined()
 
       loader.value = null
       const value2 = await operation.get('value')