diff --git a/package-lock.json b/package-lock.json index f06a34e..5b6e96a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8233,9 +8233,9 @@ } }, "node_modules/lodash": { - "version": "4.17.23", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.23.tgz", - "integrity": "sha512-LgVTMpQtIopCi79SJeDiP0TfWi5CNEc/L/aRdTh3yIvmZXTnheWpKjSZhnvMl8iXbC1tFg9gdHHDMLoV7CnG+w==", + "version": "4.18.1", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.18.1.tgz", + "integrity": "sha512-dMInicTPVE8d1e5otfwmmjlxkZoUpiVLwyeTdUsi/Caj/gfzzblBcCE5sRHV/AsjuCmxWrte2TNGSYuCeCq+0Q==", "dev": true, "license": "MIT" }, @@ -11080,7 +11080,7 @@ }, "packages/common": { "name": "@zajno/common", - "version": "2.8.10", + "version": "2.8.11", "license": "MIT", "devDependencies": { "@zajno/eslint-config": "*" @@ -11091,7 +11091,7 @@ }, "packages/common-firebase": { "name": "@zajno/common-firebase", - "version": "4.3.7", + "version": "4.3.8", "license": "MIT", "devDependencies": { "@firebase/app-types": "^0.9.3", @@ -11107,8 +11107,8 @@ }, "peerDependencies": { "@google-cloud/pubsub": "^5.2.2", - "@zajno/common": "^2.8.5", - "@zajno/common-mobx": "^1.7.3", + "@zajno/common": "~2.8.11", + "@zajno/common-mobx": "~1.7.6", "firebase": "^11.6 || ^12.8", "firebase-admin": "^12.7 || ^13.6", "firebase-functions": "^7.0.5", @@ -11132,7 +11132,7 @@ }, "packages/common-mobx": { "name": "@zajno/common-mobx", - "version": "1.7.5", + "version": "1.7.6", "license": "MIT", "devDependencies": { "@zajno/common": "*", @@ -11140,7 +11140,7 @@ "mobx": "^6.13.7" }, "peerDependencies": { - "@zajno/common": "~2.8.10", + "@zajno/common": "~2.8.11", "mobx": "^6.13", "tslib": "^2.8" } diff --git a/packages/common-firebase/package.json b/packages/common-firebase/package.json index a80091f..6827a6d 100644 --- a/packages/common-firebase/package.json +++ b/packages/common-firebase/package.json @@ -1,6 +1,6 @@ { "name": "@zajno/common-firebase", - "version": "4.3.7", + "version": "4.3.8", "description": "Zajno's re-usable Firebase utilities for JS/TS projects", "private": false, "type": "module", @@ -47,8 +47,8 @@ }, "peerDependencies": { "@google-cloud/pubsub": "^5.2.2", - "@zajno/common": "^2.8.5", - "@zajno/common-mobx": "^1.7.3", + "@zajno/common": "~2.8.11", + "@zajno/common-mobx": "~1.7.6", "firebase": "^11.6 || ^12.8", "firebase-admin": "^12.7 || ^13.6", "firebase-functions": "^7.0.5", diff --git a/packages/common-mobx/package.json b/packages/common-mobx/package.json index 7ecd69e..b00a516 100644 --- a/packages/common-mobx/package.json +++ b/packages/common-mobx/package.json @@ -1,6 +1,6 @@ { "name": "@zajno/common-mobx", - "version": "1.7.5", + "version": "1.7.6", "description": "Zajno's re-usable utilities for JS/TS projects using MobX", "private": false, "type": "module", @@ -40,7 +40,7 @@ "mobx": "^6.13.7" }, "peerDependencies": { - "@zajno/common": "~2.8.10", + "@zajno/common": "~2.8.11", "mobx": "^6.13", "tslib": "^2.8" }, diff --git a/packages/common-mobx/src/structures/__tests__/promiseCache.test.ts b/packages/common-mobx/src/structures/__tests__/promiseCache.test.ts index 5cc35bc..d839421 100644 --- a/packages/common-mobx/src/structures/__tests__/promiseCache.test.ts +++ b/packages/common-mobx/src/structures/__tests__/promiseCache.test.ts @@ -1,6 +1,6 @@ import { Disposer } from '@zajno/common/functions/disposer'; import { PromiseCacheObservable } from '../promiseCache.js'; -import { reaction, runInAction } from 'mobx'; +import { reaction, runInAction, configure } from 'mobx'; describe('PromiseCache observable', () => { beforeEach(() => { @@ -444,4 +444,134 @@ describe('PromiseCache observable', () => { disposer.dispose(); }); + + // ─── Sticky error: no infinite re-fetch loop ───────────────────────── + + it('reaction on getCurrent does NOT cause infinite fetch loop on error', async () => { + // Enforce actions to catch unprotected observable mutations + configure({ enforceActions: 'observed' }); + + const fetcher = vi.fn(async () => { + throw new Error('always fails'); + }); + + const cache = new PromiseCacheObservable(fetcher); + + // First: verify that get() after error doesn't re-fetch (non-reactive) + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + expect(cache.getLastError('a')).toBeInstanceOf(Error); + + // Second get() should NOT re-fetch + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + + // Now test with reaction + fetcher.mockClear(); + cache.clear(); // reset state + + const valueHandler = vi.fn(); + const disposer = new Disposer(); + + disposer.add( + reaction( + () => cache.getCurrent('a', false), // read without triggering fetch + v => valueHandler(v), + { fireImmediately: true }, + ), + ); + + // Manually trigger fetch + await cache.get('a'); + + expect(fetcher).toHaveBeenCalledTimes(1); + expect(cache.getLastError('a')).toBeInstanceOf(Error); + + // Subsequent get() should NOT re-fetch + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + + disposer.dispose(); + configure({ enforceActions: 'never' }); + }); + + it('reaction on getLazy().value does NOT cause infinite fetch loop on error', async () => { + const fetcher = vi.fn(async () => { + throw new Error('always fails'); + }); + + const cache = new PromiseCacheObservable(fetcher); + const lazy = cache.getLazy('a'); + + // First: verify non-reactive behavior + await lazy.promise; + expect(fetcher).toHaveBeenCalledTimes(1); + expect(lazy.error).toBeInstanceOf(Error); + + // Accessing value should NOT re-trigger fetch + void lazy.value; + expect(fetcher).toHaveBeenCalledTimes(1); + + // Now test with reaction on currentValue (read-only, no fetch trigger) + fetcher.mockClear(); + cache.clear(); + + const valueHandler = vi.fn(); + const disposer = new Disposer(); + + disposer.add( + reaction( + () => lazy.currentValue, + v => valueHandler(v), + { fireImmediately: true }, + ), + ); + + // Manually trigger fetch + await cache.get('a'); + + expect(fetcher).toHaveBeenCalledTimes(1); + expect(lazy.error).toBeInstanceOf(Error); + + // Subsequent get() should NOT re-fetch + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + + disposer.dispose(); + }); + + it('after sticky error, refresh() still works in observable context', async () => { + let callCount = 0; + const fetcher = vi.fn(async () => { + callCount++; + if (callCount === 1) throw new Error('first call fails'); + return `success-${callCount}`; + }); + + const cache = new PromiseCacheObservable(fetcher); + + const valueHandler = vi.fn(); + const disposer = new Disposer(); + + // Initial fetch fails + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + expect(cache.getLastError('a')).toBeInstanceOf(Error); + + // Set up reaction + disposer.add( + reaction( + () => cache.getCurrent('a', false), + v => valueHandler(v), + ), + ); + + // refresh() should work and trigger the reaction + const result = await cache.refresh('a'); + expect(result).toBe('success-2'); + expect(cache.getLastError('a')).toBeNull(); + expect(valueHandler).toHaveBeenCalledWith('success-2'); + + disposer.dispose(); + }); }); diff --git a/packages/common-mobx/src/structures/promiseCache.ts b/packages/common-mobx/src/structures/promiseCache.ts index 5276f9d..1236861 100644 --- a/packages/common-mobx/src/structures/promiseCache.ts +++ b/packages/common-mobx/src/structures/promiseCache.ts @@ -27,7 +27,7 @@ export class PromiseCacheObservable(this, { @@ -37,7 +37,7 @@ export class PromiseCacheObservable { - return observable.map(undefined, { deep: false }); + protected pure_createItemsCache(): IMapModel { + return observable.map(undefined, { deep: false }); } protected pure_createItemsStatus(): IMapModel { diff --git a/packages/common/package.json b/packages/common/package.json index f4fa2e6..c28c1d5 100644 --- a/packages/common/package.json +++ b/packages/common/package.json @@ -1,6 +1,6 @@ { "name": "@zajno/common", - "version": "2.8.10", + "version": "2.8.11", "description": "Zajno's re-usable utilities for JS/TS projects", "private": false, "type": "module", diff --git a/packages/common/src/lazy/__tests__/promise.refresh.test.ts b/packages/common/src/lazy/__tests__/promise.refresh.test.ts index 27786cc..31049ff 100644 --- a/packages/common/src/lazy/__tests__/promise.refresh.test.ts +++ b/packages/common/src/lazy/__tests__/promise.refresh.test.ts @@ -137,7 +137,7 @@ describe('LazyPromise refresh', () => { await lazy.promise; expect(lazy.error).toBeInstanceOf(Error); expect(lazy.errorMessage).toBe('Sync initial error'); - expect(lazy.hasValue).toBeTrue(); + expect(lazy.hasValue).toBeFalse(); expect(lazy.value).toBeUndefined(); }); diff --git a/packages/common/src/lazy/__tests__/promise.test.ts b/packages/common/src/lazy/__tests__/promise.test.ts index fc899c5..674d575 100644 --- a/packages/common/src/lazy/__tests__/promise.test.ts +++ b/packages/common/src/lazy/__tests__/promise.test.ts @@ -223,7 +223,7 @@ describe('LazyPromise', () => { await l.promise; expect(l.error).toBeInstanceOf(Error); expect(l.errorMessage).toBe('async error message'); - expect(l.hasValue).toBeTrue(); + expect(l.hasValue).toBeFalse(); expect(l.value).toBeUndefined(); } @@ -280,4 +280,47 @@ describe('LazyPromise', () => { expect(lazy.hasValue).toBeFalse(); }); + // ─── hasResolvedValue type narrowing ───────────────────────────────── + + test('hasResolvedValue narrows type after successful load', async () => { + const lazy = new LazyPromise(async () => { + await delay(10); + return { name: 'Alice' }; + }); + + expect(lazy.hasResolvedValue()).toBe(false); + + const p = lazy.promise; + await vi.advanceTimersByTimeAsync(10); + await p; + + expect(lazy.hasResolvedValue()).toBe(true); + + if (lazy.hasResolvedValue()) { + // Type narrowing: value is { name: string }, not { name: string } | undefined + const name: string = lazy.value.name; + expect(name).toBe('Alice'); + + const current: { name: string } = lazy.currentValue; + expect(current.name).toBe('Alice'); + } + }); + + test('hasResolvedValue returns false after error', async () => { + const lazy = new LazyPromise(async () => { + throw new Error('fail'); + }); + + await lazy.promise; + + expect(lazy.hasResolvedValue()).toBe(false); + expect(lazy.hasValue).toBe(false); + expect(lazy.error).toBeInstanceOf(Error); + }); + + test('hasResolvedValue returns false before load', () => { + const lazy = new LazyPromise(async () => 42); + expect(lazy.hasResolvedValue()).toBe(false); + }); + }); diff --git a/packages/common/src/lazy/promise.ts b/packages/common/src/lazy/promise.ts index 3773aa3..852f23b 100644 --- a/packages/common/src/lazy/promise.ts +++ b/packages/common/src/lazy/promise.ts @@ -2,7 +2,7 @@ import { tryDispose, type IDisposable } from '../functions/disposer.js'; import { formatError } from '../functions/safe.js'; import type { IResettableModel } from '../models/types.js'; import type { IExpireTracker } from '../structures/expire.js'; -import type { IControllableLazyPromise, ILazyPromiseExtension, LazyFactory } from './types.js'; +import type { IControllableLazyPromise, ILazyPromiseExtension, IResolvedLazyPromise, LazyFactory } from './types.js'; /** * Asynchronous lazy-loading container that initializes via a promise-based factory. @@ -42,9 +42,16 @@ export class LazyPromise implemen /** Current loading state: true = loading, false = loaded, null = not started */ public get isLoading() { return this._state; } - public get hasValue() { return this._state === false; } + + /** Returns true if a value of type `T` has been successfully loaded (no error). */ + public get hasValue() { return this._state === false && this._error == null; } public get error(): unknown { return this._error; } + /** @inheritdoc */ + public hasResolvedValue(): this is LazyPromise & IResolvedLazyPromise { + return this._state === false && this._error == null; + } + /** @deprecated Use {@link error} instead. */ public get errorMessage(): string | null { return this._error != null ? formatError(this._error) : null; diff --git a/packages/common/src/lazy/types.ts b/packages/common/src/lazy/types.ts index af16e28..62ff80f 100644 --- a/packages/common/src/lazy/types.ts +++ b/packages/common/src/lazy/types.ts @@ -5,7 +5,14 @@ export interface ILazy { /** Returns current value, triggering loading if not yet loaded. */ readonly value: T; - /** Returns true if value has been loaded. Does not trigger loading. */ + /** + * Returns true if a value of type `T` has been successfully loaded (no error). + * + * When `true`, `value` is guaranteed to be `T` (not `TInitial` or an error fallback). + * When `false`, `value` may be `TInitial`, `undefined`, or a stale value from a previous successful load. + * + * Does not trigger loading. + */ readonly hasValue: boolean; /** Returns current value or undefined if not loaded. Does not trigger loading. */ @@ -60,6 +67,31 @@ export interface ILazyPromise ext * @returns Promise resolving to the refreshed value, or the current value on error */ refresh(): Promise; + + /** + * Type-narrowing check: returns `true` if the value has been successfully resolved to `T`. + * + * When this returns `true`, `value` and `currentValue` are narrowed to `T` (not `TInitial`). + * + * @example + * ```typescript + * const lazy: ILazyPromise = cache.getLazy('user-1'); + * if (lazy.hasResolvedValue()) { + * // lazy.value is `User` here, not `User | undefined` + * console.log(lazy.value.name); + * } + * ``` + */ + hasResolvedValue(): this is IResolvedLazyPromise; +} + +/** Narrowed state of ILazyPromise after successful resolution. */ +export interface IResolvedLazyPromise extends ILazyPromise { + readonly value: T; + readonly currentValue: T; + readonly hasValue: true; + readonly isLoading: false; + readonly error: null; } /** diff --git a/packages/common/src/structures/promiseCache/__tests__/errors.test.ts b/packages/common/src/structures/promiseCache/__tests__/errors.test.ts index 2d6d1c6..c04ac6d 100644 --- a/packages/common/src/structures/promiseCache/__tests__/errors.test.ts +++ b/packages/common/src/structures/promiseCache/__tests__/errors.test.ts @@ -111,6 +111,140 @@ describe('PromiseCache errors', () => { }); }); + describe('sticky error (no infinite re-fetch loop)', () => { + test('get() after failure does NOT re-fetch — error is sticky', async () => { + const fetcher = vi.fn(async () => { + throw new Error('always fails'); + }); + + const cache = new PromiseCache(fetcher); + + // First get() — triggers fetch, which fails + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + expect(cache.getLastError('a')).toBeInstanceOf(Error); + + fetcher.mockClear(); + + // Second get() — should NOT trigger another fetch + const result = await cache.get('a'); + expect(fetcher).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + expect(cache.getLastError('a')).toBeInstanceOf(Error); + }); + + test('getCurrent(key, true) after failure does NOT re-fetch', async () => { + const fetcher = vi.fn(async () => { + throw new Error('always fails'); + }); + + const cache = new PromiseCache(fetcher); + + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + fetcher.mockClear(); + + // getCurrent with initiateFetch=true should NOT re-trigger + cache.getCurrent('a', true); + expect(fetcher).not.toHaveBeenCalled(); + }); + + test('with useInitialValue, get() after failure returns initial value without re-fetching', async () => { + const fetcher = vi.fn(async () => { + throw new Error('always fails'); + }); + + const cache = new PromiseCache(fetcher) + .useInitialValue('fallback'); + + await cache.get('a'); + expect(fetcher).toHaveBeenCalledTimes(1); + fetcher.mockClear(); + + const result = await cache.get('a'); + expect(fetcher).not.toHaveBeenCalled(); + expect(result).toBe('fallback'); + }); + + test('refresh() after sticky error DOES re-fetch', async () => { + let callCount = 0; + const cache = new PromiseCache(async () => { + callCount++; + if (callCount === 1) throw new Error('first call fails'); + return 'success'; + }); + + // First get() fails + await cache.get('a'); + expect(callCount).toBe(1); + expect(cache.getLastError('a')).toBeInstanceOf(Error); + + // refresh() should re-fetch even after sticky error + const result = await cache.refresh('a'); + expect(callCount).toBe(2); + expect(result).toBe('success'); + expect(cache.getLastError('a')).toBeNull(); + }); + + test('invalidate() + get() after error DOES re-fetch', async () => { + let callCount = 0; + const cache = new PromiseCache(async () => { + callCount++; + if (callCount === 1) throw new Error('first call fails'); + return 'success'; + }); + + // First get() fails + await cache.get('a'); + expect(callCount).toBe(1); + expect(cache.getLastError('a')).toBeInstanceOf(Error); + + // invalidate() resets the error state + cache.invalidate('a'); + expect(cache.getLastError('a')).toBeNull(); + + // Now get() should re-fetch + const result = await cache.get('a'); + expect(callCount).toBe(2); + expect(result).toBe('success'); + }); + + test('multiple get() calls on consistently failing fetcher — fetcher called only once', async () => { + const fetcher = vi.fn(async () => { + throw new Error('always fails'); + }); + + const cache = new PromiseCache(fetcher); + + // Call get() multiple times + await cache.get('a'); + await cache.get('a'); + await cache.get('a'); + await cache.get('a'); + await cache.get('a'); + + // Fetcher should have been called only once + expect(fetcher).toHaveBeenCalledTimes(1); + }); + + test('getLazy().value after failure does NOT re-fetch', async () => { + const fetcher = vi.fn(async () => { + throw new Error('always fails'); + }); + + const cache = new PromiseCache(fetcher); + + const lazy = cache.getLazy('a'); + await lazy.promise; + expect(fetcher).toHaveBeenCalledTimes(1); + fetcher.mockClear(); + + // Accessing .value should NOT trigger a new fetch + void lazy.value; + expect(fetcher).not.toHaveBeenCalled(); + }); + }); + describe('onError callback', () => { test('calls onError when fetcher fails', async () => { const fetchError = new Error('Fetch failed'); diff --git a/packages/common/src/structures/promiseCache/cache.ts b/packages/common/src/structures/promiseCache/cache.ts index 57ef44c..f8542e8 100644 --- a/packages/common/src/structures/promiseCache/cache.ts +++ b/packages/common/src/structures/promiseCache/cache.ts @@ -117,10 +117,11 @@ export class PromiseCache { - const { item, key, isInvalid } = this._getCurrent(id); + const key = this._pk(id); // return cached item if it's not invalidated - if (item !== undefined && !isInvalid) { + if (this._itemsCache.has(key) && !this.getIsInvalidated(key)) { + const item = this._itemsCache.get(key)!; this.logger.log(key, 'get: item resolved to', item); return Promise.resolve(item); } @@ -132,6 +133,14 @@ export class PromiseCache this._itemsCache.get(key) ?? this._getInitialValue(id)) as Promise; + return newerPromise.catch(() => this._getCachedOrInitial(key, id)); } // Fallback: return current cached value or initial - return this._itemsCache.get(key) ?? this._getInitialValue(id); + return this._getCachedOrInitial(key, id); } // We are the latest — clean up tracking this._activeFetchPromises.delete(key); - if (!fetchFailed && res !== undefined) { + if (fetchResult.ok) { + const res = this.prepareResult(fetchResult.value); this.logger.log(key, 'item\'s resolved to', res); - res = this.prepareResult(res); this.storeResult(key, res); - } else if (fetchFailed) { - // Keep stale value — return whatever is in cache, or initial value - return this._itemsCache.get(key) ?? this._getInitialValue(id); + return res; } - return res ?? this._getInitialValue(id); + // Fetch failed — record timestamp for time-based invalidation expiry, + // and return stale value or initial. + this._timestamps.set(key, Date.now()); + return this._getCachedOrInitial(key, id); } finally { if (!isInSameVersion) { this.logger.log(key, 'skipping item\'s resolve due to version change ("clear()" has been called)'); @@ -376,7 +367,7 @@ export class PromiseCache extends Loggable { /** Stores resolved items in map by id. */ - protected readonly _itemsCache: IMapModel; + protected readonly _itemsCache: IMapModel; /** Stores items loading state (loading or not) in map by id. */ protected readonly _itemsStatus: IMapModel; @@ -107,8 +107,8 @@ export abstract class PromiseCacheCore { - return new Map(); + protected pure_createItemsCache(): IMapModel { + return new Map(); } /** @@ -179,12 +179,12 @@ export abstract class PromiseCacheCore { // eslint-disable-next-line @typescript-eslint/no-this-alias const self = this; - return { + const lazy: ILazyPromise = { get value() { return self.getCurrent(key); }, get currentValue() { return self.getCurrent(key, false); }, get hasValue() { const k = self._pk(key); - return self._itemsCache.has(k); + return self._itemsCache.has(k) && !self._errorsMap.has(k); }, get error() { return self.getLastError(key); }, /** @deprecated Use {@link error} instead. */ @@ -200,7 +200,11 @@ export abstract class PromiseCacheCore): this is IResolvedLazyPromise { + return lazy.hasValue; + }, }; + return lazy; } /** @@ -248,10 +252,7 @@ export abstract class PromiseCacheCore | undefined, isLoading: boolean | undefined) { - PromiseCacheCore._setMapX(key, this._fetchCache, promise); - PromiseCacheCore._setMapX(key, this._itemsStatus, isLoading); - PromiseCacheCore._setMapX(key, this._itemsCache, item); + /** @internal Deletes all cache entries for a key (item, promise, status). */ + protected _deleteKey(key: string) { + this._fetchCache.delete(key); + this._itemsStatus.delete(key); + this._itemsCache.delete(key); } /** Updates the loading status for the specified key. Override to add a hook. */ @@ -462,12 +468,4 @@ export abstract class PromiseCacheCore(key: string, map: IMapModel, val: T) { - if (val === undefined) { - map.delete(key); - } else { - map.set(key, val); - } - } }