-
Notifications
You must be signed in to change notification settings - Fork 274
feat: implement cacheForRequest() per-request factory cache #646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /** | ||
| * Public entry for per-request caching utilities. | ||
| * | ||
| * @example | ||
| * ```ts | ||
| * import { cacheForRequest } from "vinext/cache"; | ||
| * ``` | ||
| * | ||
| * @module | ||
| */ | ||
| export { cacheForRequest } from "./shims/cache-for-request.js"; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,89 @@ | ||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Cache a factory function's result for the duration of a request. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * Returns a function that lazily invokes the factory on first call within | ||||||||||||||||||||
| * a request, then returns the cached result for all subsequent calls in | ||||||||||||||||||||
| * the same request. Each new request gets a fresh invocation. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * The factory function's identity (reference) is the cache key — no | ||||||||||||||||||||
| * string keys, no collision risk between modules. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * Async factories are supported: the returned Promise is cached, so | ||||||||||||||||||||
| * concurrent `await` calls within the same request share one invocation. | ||||||||||||||||||||
| * If the Promise rejects, the cached entry is cleared so the next call | ||||||||||||||||||||
| * can retry. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * Outside a request scope (tests, build-time), the factory runs every | ||||||||||||||||||||
| * time with no caching — safe and predictable. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * @example | ||||||||||||||||||||
| * ```ts | ||||||||||||||||||||
| * import { cacheForRequest } from "vinext/cache"; | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * const getPrisma = cacheForRequest(() => { | ||||||||||||||||||||
| * const pool = new Pool({ connectionString: env.HYPERDRIVE.connectionString }); | ||||||||||||||||||||
| * return new PrismaClient({ adapter: new PrismaPg(pool) }); | ||||||||||||||||||||
| * }); | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * // In a route handler or server component: | ||||||||||||||||||||
| * const prisma = getPrisma(); // first call creates, subsequent calls reuse | ||||||||||||||||||||
| * ``` | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * @example | ||||||||||||||||||||
| * ```ts | ||||||||||||||||||||
| * // Async factory — Promise is cached, not re-invoked. | ||||||||||||||||||||
| * // If it rejects, the cache is cleared for retry. | ||||||||||||||||||||
| * const getDb = cacheForRequest(async () => { | ||||||||||||||||||||
| * const pool = new Pool({ connectionString }); | ||||||||||||||||||||
| * await pool.connect(); | ||||||||||||||||||||
| * return drizzle(pool); | ||||||||||||||||||||
| * }); | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * const db = await getDb(); | ||||||||||||||||||||
| * ``` | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * @module | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import { getRequestContext, isInsideUnifiedScope } from "./unified-request-context.js"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Create a request-scoped cached version of a factory function. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * @param factory - Function that creates the value. Called once per request for sync | ||||||||||||||||||||
| * factories. Async factories that reject have their cache cleared, allowing retry. | ||||||||||||||||||||
| * @returns A function with the same return type that caches the result per request. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| export function cacheForRequest<T>(factory: () => T): () => T { | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation is clean and correct. One thing to consider: if a factory throws, the error is not cached — subsequent calls will re-invoke the factory. This is actually good behavior (retry-on-error), but it's worth documenting explicitly since the async case differs subtly: a rejected Promise will be cached (because For async factories that can fail, users would get a permanently cached rejected Promise within that request. Consider whether you want to handle this: const value = factory();
if (value instanceof Promise) {
// Self-healing: delete from cache if the promise rejects
value.catch(() => cache.delete(factory));
}
cache.set(factory, value);This is a design decision rather than a bug — either behavior is defensible. But the current JSDoc says "the returned Promise is cached, so concurrent |
||||||||||||||||||||
| return (): T => { | ||||||||||||||||||||
| if (!isInsideUnifiedScope()) { | ||||||||||||||||||||
| return factory(); | ||||||||||||||||||||
|
Comment on lines
+59
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const ctx = getRequestContext(); | ||||||||||||||||||||
| const cache = ctx.requestCache; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (cache.has(factory)) { | ||||||||||||||||||||
| return cache.get(factory) as T; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const value = factory(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // For async factories: if the Promise rejects, clear the cached entry | ||||||||||||||||||||
| // so subsequent calls within the same request can retry. | ||||||||||||||||||||
| if (value instanceof Promise) { | ||||||||||||||||||||
| cache.set(factory, value); | ||||||||||||||||||||
| (value as Promise<unknown>).catch(() => { | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the
Suggested change
|
||||||||||||||||||||
| // Only clear if the cached value is still this exact Promise | ||||||||||||||||||||
| // (avoids clearing a newer retry's value). | ||||||||||||||||||||
| if (cache.get(factory) === value) { | ||||||||||||||||||||
| cache.delete(factory); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| cache.set(factory, value); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return value; | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,108 @@ | ||||||||||||||||||||||||||
| import { describe, it, expect, vi } from "vite-plus/test"; | ||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||
| runWithRequestContext, | ||||||||||||||||||||||||||
| runWithUnifiedStateMutation, | ||||||||||||||||||||||||||
| createRequestContext, | ||||||||||||||||||||||||||
| } from "../packages/vinext/src/shims/unified-request-context"; | ||||||||||||||||||||||||||
| import { cacheForRequest } from "../packages/vinext/src/shims/cache-for-request"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| describe("cacheForRequest", () => { | ||||||||||||||||||||||||||
| it("does not cache outside request scope", () => { | ||||||||||||||||||||||||||
| const factory = vi.fn(() => ({ id: Math.random() })); | ||||||||||||||||||||||||||
| const get = cacheForRequest(factory); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const a = get(); | ||||||||||||||||||||||||||
| const b = get(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(a).not.toBe(b); | ||||||||||||||||||||||||||
| expect(factory).toHaveBeenCalledTimes(2); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it("caches within the same request", () => { | ||||||||||||||||||||||||||
| const factory = vi.fn(() => ({ id: Math.random() })); | ||||||||||||||||||||||||||
| const get = cacheForRequest(factory); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ctx = createRequestContext(); | ||||||||||||||||||||||||||
| runWithRequestContext(ctx, () => { | ||||||||||||||||||||||||||
| const a = get(); | ||||||||||||||||||||||||||
| const b = get(); | ||||||||||||||||||||||||||
| expect(a).toBe(b); | ||||||||||||||||||||||||||
| expect(factory).toHaveBeenCalledTimes(1); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it("caches different factories separately", () => { | ||||||||||||||||||||||||||
| const factoryA = vi.fn(() => "a"); | ||||||||||||||||||||||||||
| const factoryB = vi.fn(() => "b"); | ||||||||||||||||||||||||||
| const getA = cacheForRequest(factoryA); | ||||||||||||||||||||||||||
| const getB = cacheForRequest(factoryB); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ctx = createRequestContext(); | ||||||||||||||||||||||||||
| runWithRequestContext(ctx, () => { | ||||||||||||||||||||||||||
| expect(getA()).toBe("a"); | ||||||||||||||||||||||||||
| expect(getB()).toBe("b"); | ||||||||||||||||||||||||||
| expect(factoryA).toHaveBeenCalledTimes(1); | ||||||||||||||||||||||||||
| expect(factoryB).toHaveBeenCalledTimes(1); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it("isolates between different requests", () => { | ||||||||||||||||||||||||||
| let counter = 0; | ||||||||||||||||||||||||||
| const factory = vi.fn(() => ++counter); | ||||||||||||||||||||||||||
| const get = cacheForRequest(factory); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ctx1 = createRequestContext(); | ||||||||||||||||||||||||||
| const val1 = runWithRequestContext(ctx1, () => get()); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ctx2 = createRequestContext(); | ||||||||||||||||||||||||||
| const val2 = runWithRequestContext(ctx2, () => get()); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| expect(val1).toBe(1); | ||||||||||||||||||||||||||
| expect(val2).toBe(2); | ||||||||||||||||||||||||||
| expect(factory).toHaveBeenCalledTimes(2); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it("shares cache across nested unified scopes", async () => { | ||||||||||||||||||||||||||
| const factory = vi.fn(() => "cached"); | ||||||||||||||||||||||||||
| const get = cacheForRequest(factory); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ctx = createRequestContext(); | ||||||||||||||||||||||||||
| await runWithRequestContext(ctx, async () => { | ||||||||||||||||||||||||||
| const outer = get(); | ||||||||||||||||||||||||||
| // Exercise the real nested scope path via runWithUnifiedStateMutation | ||||||||||||||||||||||||||
| const inner = await runWithUnifiedStateMutation( | ||||||||||||||||||||||||||
| (child) => { | ||||||||||||||||||||||||||
| child.dynamicUsageDetected = true; | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| () => get(), | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
|
Comment on lines
+69
to
+78
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test manually constructs Consider using
Suggested change
This also means adding |
||||||||||||||||||||||||||
| expect(outer).toBe("cached"); | ||||||||||||||||||||||||||
| expect(inner).toBe("cached"); | ||||||||||||||||||||||||||
| expect(factory).toHaveBeenCalledTimes(1); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| it("caches async Promise and clears on rejection", async () => { | ||||||||||||||||||||||||||
| let callCount = 0; | ||||||||||||||||||||||||||
| const factory = vi.fn(async () => { | ||||||||||||||||||||||||||
| callCount++; | ||||||||||||||||||||||||||
| if (callCount === 1) throw new Error("fail"); | ||||||||||||||||||||||||||
| return "success"; | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| const get = cacheForRequest(factory); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ctx = createRequestContext(); | ||||||||||||||||||||||||||
| await runWithRequestContext(ctx, async () => { | ||||||||||||||||||||||||||
| // First call: rejects | ||||||||||||||||||||||||||
| await expect(get()).rejects.toThrow("fail"); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Wait a tick for the .catch() to clear the cache | ||||||||||||||||||||||||||
| await new Promise((r) => setTimeout(r, 0)); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Second call: should retry (cache was cleared) | ||||||||||||||||||||||||||
| const result = await get(); | ||||||||||||||||||||||||||
| expect(result).toBe("success"); | ||||||||||||||||||||||||||
| expect(factory).toHaveBeenCalledTimes(2); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import uses a relative path (
./unified-request-context.js), which is correct for the shims directory. However, the Vite plugin'sresolve.aliasmap (index.ts:1653-1663) does not include a"vinext/cache-for-request"entry.All other
vinext/*shims are registered in that alias map so they resolve correctly during dev. Without this entry,import { cacheForRequest } from "vinext/cache-for-request"in user code will fail duringvite dev.Needs a corresponding entry:
in the alias map alongside the other
vinext/*entries.