Skip to content

feat(cache): configure cache adapters from vite plugin config#1733

Merged
james-elicx merged 16 commits into
mainfrom
claude/sweet-germain-225293
Jun 4, 2026
Merged

feat(cache): configure cache adapters from vite plugin config#1733
james-elicx merged 16 commits into
mainfrom
claude/sweet-germain-225293

Conversation

@james-elicx
Copy link
Copy Markdown
Member

@james-elicx james-elicx commented Jun 4, 2026

What

Adds a cache option to the vinext() plugin so CDN and data cache adapters can be configured declaratively in vite.config, instead of needing a custom worker entry that calls setDataCacheHandler() / setCdnCacheAdapter():

import vinext from "vinext";
import { cdnAdapter } from "vinext/cloudflare/cache/cdn-adapter";
import { kvDataAdapter } from "vinext/cloudflare/cache/kv-data-adapter";

vinext({
  cache: {
    cdn:  cdnAdapter(),
    data: kvDataAdapter({ binding: "MY_KV" }), // defaults to "VINEXT_CACHE"
  },
})

Design

Config time vs. request time. A builder runs in vite.config — plain Node, no Workers context. It only returns a serializable descriptor { adapter, options }; it never imports the handler or reads a binding, so nothing throws at build/dev time. The builder require.resolves its sibling runtime factory (import.meta.resolve + fileURLToPath, the ESM form of require.resolve) so the descriptor carries an absolute path, decoupled from package export wiring. On the first request the generated virtual:vinext-cache-adapters module imports that factory and calls it with { env, options } — the binding is resolved then (env[binding]).

Adapter layout. Each adapter is a builder module + a sibling *.runtime.ts holding the default-export factory. Types live in one home (vinext/shims/cache-adapter): CacheAdapterDescriptor, VinextCacheConfig, and the { env, options } factory/context types.

Registered across every router and runtime — not just one entry:

  • App Router: the generated RSC entry passes registerConfiguredCacheAdapters into createAppRscHandler, which calls it per request — covering Workers, the Node server (vinext start), and dev through the one shared handler. The Workers entry also registers with env first so KV bindings resolve.
  • Pages Router: the generated server entry registers in renderPage + handleApiRoute (Node/dev), and the generated worker registers with env (Workers/KV).

Registration self-guards (instantiated once per isolate; the first call with real env wins) and is resilient — a factory that can't initialize on a given runtime (e.g. a KV binding that doesn't exist on the Node server) is logged and skipped, leaving the default handler in place rather than failing requests.

Cloudflare adapters (new vinext/cloudflare/cache/* subpaths)

  • kvDataAdapter({ binding?, appPrefix?, ttlSeconds?, tagCacheTtlMs? })KVCacheHandler (default binding VINEXT_CACHE)
  • cdnAdapter()CloudflareCdnCacheAdapter (edge-managed page ISR)

The raw { adapter: require.resolve("./my-adapter.js"), options } form still works — builders are sugar over it. Explicit CDN registration takes precedence over VINEXT_CDN_CACHE_AUTO_DETECT; the imperative setDataCacheHandler() / setCdnCacheAdapter() API is unchanged.

Tests

  • tests/cache-adapters-config.test.ts — codegen permutations + inlined options; the kvDataAdapter / cdnAdapter builders (absolute-path resolution, config-time validation); runtime factories (custom-binding + missing-binding errors); and generator-level assertions that every router/runtime entry wires registration.
  • tests/cache-adapters-build.test.ts — real Cloudflare build pointing cache.data at a local adapter by absolute path (what require.resolve("./…") yields), proving it resolves and bundles with no build-time Workers context.
  • tests/tsconfig-path-alias-build.test.ts (existing) builds a worker through app-router-entry.ts, exercising the virtual module end-to-end.
  • vp check clean; app-router (339) and pages-router (272) suites pass.

Add a `cache` option to the vinext() plugin so CDN and data cache
adapters can be declared in vite.config instead of calling
setDataCacheHandler() / setCdnCacheAdapter() from a worker entry:

  vinext({
    cache: {
      cdn:  { adapter: require.resolve('vinext/cloudflare/cache/cdn-adapter') },
      data: { adapter: require.resolve('vinext/cloudflare/cache/kv-data-adapter') },
    },
  })

Each slot points at an adapter module whose default export is a factory
(DataCacheAdapterFactory / CdnCacheAdapterFactory). The plugin generates
a virtual:vinext-cache-adapters module that the App Router worker entry
calls per request (self-guarded, once per isolate), passing the host env
so binding-backed adapters (e.g. KV) can read their namespace.

Ships ready-made Cloudflare adapter entry points:
  - vinext/cloudflare/cache/kv-data-adapter  (KVCacheHandler)
  - vinext/cloudflare/cache/cdn-adapter      (CloudflareCdnCacheAdapter)
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Jun 4, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1733

commit: 6f444e0

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 4, 2026

Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

Instead of `{ adapter: require.resolve(...) }`, each adapter module now
also exports a config-time builder from the same path:

  import { cdnAdapter } from 'vinext/cloudflare/cache/cdn-adapter';
  import { kvDataAdapter } from 'vinext/cloudflare/cache/kv-data-adapter';

  vinext({ cache: { cdn: cdnAdapter(), data: kvDataAdapter({ binding: 'MY_KV' }) } })

A builder returns a plain, serializable { adapter, options } descriptor —
it never touches the Workers runtime, so nothing throws at config / build
/ dev time when bindings aren't available. Descriptor `options` (e.g. the
KV binding name) are inlined into the generated registration module and
forwarded to the factory's { env, options } context, where the binding is
resolved lazily on the first request.

- shims/cache-adapter: descriptors + options-aware factory/context types
- kv-data-adapter: kvDataAdapter() builder + configurable binding/appPrefix/ttl
- cdn-adapter: cdnAdapter() builder
- raw { adapter, options } path form still supported
…dles

Real Cloudflare build pointing cache.data at a local adapter file by
absolute path (what require.resolve('./adapter') yields). Proves the
generated registration module resolves the absolute import, bundles the
local adapter into the worker, and does not need any Workers context at
build time.
…s/runtimes

Addresses review feedback:

* Move adapters into their own runtime modules instead of re-exporting.
  Each adapter is now a builder module (kv-data-adapter.ts / cdn-adapter.ts)
  plus a sibling *.runtime.ts holding the default-export factory. Type
  definitions have a single home in shims/cache-adapter.ts (dropped the
  re-export shim; index.ts imports the config type from there).

* The exposed builder utility resolves the relative runtime path internally
  via import.meta.resolve (the ESM require.resolve), so the descriptor carries
  an absolute path to the runtime factory rather than a bare specifier — the
  example is just kvDataAdapter({ binding }), no require.resolve at the call site.

* Register configured cache handlers EVERYWHERE, not just the App Router worker:
  - App Router: the generated RSC entry passes registerConfiguredCacheAdapters
    into createAppRscHandler, which calls it per request — covering Workers,
    the Node server, and dev through the one shared handler.
  - Pages Router: the generated server entry registers in renderPage and
    handleApiRoute (Node/dev), and the generated worker registers with env
    (Workers, for KV bindings).
  Registration self-guards (first call with real env wins) and is now resilient:
  a factory that throws on an incompatible runtime is logged and skipped, so the
  default handler stays in place instead of failing every request.

Tests: generator-level assertions that every router/runtime entry wires
registration, plus the existing builder/codegen/factory and full-build coverage.
vp check clean; app-router (339) and pages-router (272) suites pass.
The adapter factory contract lived in shims/cache-adapter.ts (outside
cloudflare/), and the Cloudflare adapters reached out to it. Move the
contract into cloudflare/cache/adapter.ts so every Cloudflare-specific
cache adapter file is self-contained under cloudflare/ — importing only
cloudflare-local modules and the core CacheHandler/CdnCacheAdapter
interfaces it implements.

The plugin's config schema (CacheAdapterDescriptor / VinextCacheConfig)
is genuinely framework-level (it's the vinext() `cache` option), so it
moves into the codegen module the plugin already owns; index.ts imports
it from there. Builders return a structural { adapter, options } so they
don't import the descriptor type either. Deletes shims/cache-adapter.ts.
All Cloudflare cache code now lives in one directory, cloudflare/cache/,
and each runtime file holds both the implementation class and its
config-driven factory (no separate class module to reach for):

  - kv-cache-handler.ts        -> cache/kv-data-adapter.runtime.ts
    (KVCacheHandler + ENTRY_PREFIX + createKvDataCacheAdapter default export)
  - cloudflare-cdn-cache.ts    -> cache/cdn-adapter.runtime.ts
    (CloudflareCdnCacheAdapter + createCloudflareCdnCacheAdapter default export)

Updated importers: cloudflare/index.ts re-exports the classes from the
runtime files, tpr.ts pulls ENTRY_PREFIX from there, shims/cdn-cache.ts
imports the edge adapter from there, and the tests follow the moved paths.
git mv preserves history.

vp check clean; cache/kv/cdn/app-route/tpr/shims suites pass (1300+ tests).
Comment thread packages/vinext/src/cloudflare/cache/kv-data-adapter.runtime.ts Outdated
Comment thread packages/vinext/src/cloudflare/cache/adapter.ts Outdated
Comment thread packages/vinext/src/cache/cache-adapters-virtual.ts
Remove narrating/redundant comments that just restated the code; keep
the non-obvious why (registration ordering/resilience, import.meta.resolve
rationale, edge cache-control semantics). No code changes.
Comment thread packages/vinext/src/cloudflare/index.ts Outdated
Comment thread packages/vinext/src/server/app-rsc-handler.ts Outdated
Comment thread packages/vinext/src/cache/cache-adapters-virtual.ts
Comment thread packages/vinext/src/virtual-vinext-cache-adapters.d.ts Outdated
Comment thread packages/vinext/package.json
- Make registerCacheAdapters a required field on the RSC handler options
  (the generated entry already passes it; test factory updated).
- Remove the separate cloudflare/cache/adapter.ts contract file; inline the
  factory param types directly into the two runtime adapters.
- Drop the CloudflareCdnCacheAdapter re-export from cloudflare/index.ts.
- Fold the virtual:vinext-cache-adapters declaration into global.d.ts and
  delete the standalone .d.ts.
- Remove the ./cloudflare/cache/* package.json export for now; README uses a
  local-adapter require.resolve example with a note that the built-in adapter
  export paths are pending.
- Rename the config-driven KV default binding to VINEXT_KV_CACHE (imperative
  deploy/tpr path keeps VINEXT_CACHE — flagged on the thread).
Rename the KV cache binding from VINEXT_CACHE to VINEXT_KV_CACHE across the
whole codebase so the config-driven adapter, the imperative deploy-generated
worker, TPR's wrangler detection, and the apps/web example all agree. The
unrelated X-Vinext-Cache response-header constant (VINEXT_CACHE_HEADER) is
untouched.
@james-elicx
Copy link
Copy Markdown
Member Author

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(cache): configure cache adapters from vite plugin config

Solid, well-tested feature. The config-time-vs-request-time split is clean: builders return inert serializable descriptors and never touch the Workers runtime, with the binding lookup deferred to the first request. Registration is wired consistently across App Router (RSC handler chokepoint) and Pages Router (renderPage/handleApiRoute + worker entry with env), and the build test proves an absolute-path adapter bundles with no build-time Workers context. vp check + the cache/kv/cdn/deploy/app-rsc-handler suites all pass locally.

A few things worth addressing before merge:

1. VINEXT_CACHEVINEXT_KV_CACHE rename silently breaks TPR for existing deployments (blocking)

The rename was applied consistently to generated wrangler config, generated worker entry, apps/web, and docs — and apps/web/wrangler.jsonc correctly keeps the same namespace id (KV is keyed by id, not binding name), so no cache data is lost there.

The problem is parseWranglerConfig in cloudflare/tpr.ts:197 (and the TOML path at tpr.ts:276) now matches only VINEXT_KV_CACHE, with no fallback:

ns.binding === "VINEXT_KV_CACHE"

Anyone who deployed with the previous VINEXT_CACHE binding name and upgrades will have TPR silently skip with "no VINEXT_KV_CACHE KV namespace configured" (tpr.ts:810) even though their cache is wired correctly. Their app keeps working (user code is unchanged), but targeted prerender revalidation stops without an obvious reason.

Suggest one of:

  • Accept both VINEXT_KV_CACHE and the legacy VINEXT_CACHE binding name in parseWranglerConfig (prefer the new one, fall back to the old), or
  • Call this out explicitly as a breaking change in the PR description / changelog with a migration note (rename the binding in wrangler.jsonc, keep the same id).

A fallback is cheap and avoids a confusing silent regression for existing users.

2. Stale doc reference (minor)

README.md:685 still points at the old file:

cloudflare/
  kv-cache-handler.ts # Cloudflare KV-backed CacheHandler for ISR

That file was renamed to cloudflare/cache/kv-data-adapter.runtime.ts (and cloudflare-cdn-cache.tscloudflare/cache/cdn-adapter.runtime.ts). The same stale path also appears in .opencode/agents/nextjs-tracker.md:62. Worth updating so the source map matches reality.

3. inlineOptions error message overpromises (minor / doc nit)

cache/cache-adapters-virtual.ts:55-64 claims it "throws a clear config-time error if options are not serializable", but JSON.stringify only throws on circular refs / BigInt — functions, undefined, and symbols are silently dropped, not rejected. Since options is documented as JSON-serializable this is acceptable, but the comment/error implies stronger validation than it provides. Either tighten the wording or note that non-JSON values are dropped.

4. Registration guard ordering (note, not a bug)

In the generated module, __vinextCacheAdaptersRegistered = true is set before the per-adapter try blocks run. This is correct for the documented "first call with real env wins" semantics because every Workers entry calls registerConfiguredCacheAdapters(env) before the no-env call inside the shared handler (verified in app-router-entry.ts:72 and deploy.ts:609). Just flagging that this invariant — env-first on every Workers runtime — is load-bearing; if a future runtime ever invokes the handler's no-env registerCacheAdapters() before the entry's env call, a binding-backed adapter would be permanently skipped on that isolate. A short comment in the generator asserting that ordering requirement would help future maintainers.

Nothing else stands out — the cross-environment wiring, the absolute-path resolution via import.meta.resolve, and the resilient skip-on-failure behavior all look right. Items 2–4 are minor; item 1 is the one I'd want resolved (or explicitly documented) before merge.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Jun 4, 2026

Review posted to PR #1733.

Summary

I reviewed PR #1733 (feat(cache): configure cache adapters from vite plugin config), verified the changes by reading the full diff, running the relevant test suites (cache-adapters-config, kv-cache-handler, cloudflare-cdn-cache, deploy, app-rsc-handler), and running vp check. All passed.

The feature is well-designed and well-tested. I posted a review with four findings:

  1. Blocking — VINEXT_CACHEVINEXT_KV_CACHE rename silently breaks TPR for existing deployments. parseWranglerConfig (tpr.ts:197/:276) now matches only the new binding name with no fallback, so anyone who deployed with the old binding gets TPR silently skipped after upgrading. apps/web was handled correctly (same namespace id preserved, so no data loss), but the parser needs a legacy fallback or an explicit documented breaking-change/migration note.

  2. Minor — stale docs: README.md:685 and .opencode/agents/nextjs-tracker.md:62 still reference the renamed kv-cache-handler.ts path.

  3. Minor — inlineOptions comment overpromises: JSON.stringify doesn't actually reject all non-serializable inputs (functions/undefined/symbols are silently dropped).

  4. Note — registration guard ordering: the env-first-on-every-Workers-runtime invariant is load-bearing and worth a comment in the generator.

Items 2–4 are minor; item 1 is the one I'd want resolved or explicitly documented before merge.

github run

@james-elicx james-elicx force-pushed the claude/sweet-germain-225293 branch from 10af8d4 to 2dcbb59 Compare June 4, 2026 21:43
@james-elicx james-elicx force-pushed the claude/sweet-germain-225293 branch from 2dcbb59 to fac1dcf Compare June 4, 2026 21:44
@james-elicx
Copy link
Copy Markdown
Member Author

/bigbonk review

@james-elicx james-elicx marked this pull request as ready for review June 4, 2026 21:45
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: feat(cache): configure cache adapters from vite plugin config

Picking up from the prior review. The earlier findings are mostly handled:

  • #1 (legacy VINEXT_CACHE fallback) — addressed: parseWranglerConfig now matches both VINEXT_KV_CACHE and the legacy VINEXT_CACHE in both the JSON (tpr.ts:197-198) and TOML (tpr.ts:277) paths. apps/web/wrangler.jsonc preserves the same namespace id, so no data loss. Good.
  • #4 (env-first ordering invariant) — addressed: clear comments now document it at deploy.ts:608 and app-rsc-handler.ts:792-795.
  • #2 (README) — the README source-map references are updated.

But the fix for #1 introduced a new crash, and a couple of minor items are still open.

1. kv_namespaces containing a non-object entry now throws (blocking)

The legacy-fallback rewrite at cloudflare/tpr.ts:195-199 dropped the null-guard for the VINEXT_CACHE branch. Due to &&/|| precedence it parses as:

(ns && typeof ns === "object" && ns.binding === "VINEXT_KV_CACHE") || (ns.binding === "VINEXT_CACHE")

When the first branch is falsy because ns is null (or otherwise not an object), JS evaluates ns.binding === "VINEXT_CACHE" and throws:

TypeError: Cannot read properties of null (reading 'binding')

The original code (main) guarded the whole predicate with ns && typeof ns === "object" precisely to avoid this. Reproduced directly against the predicate:

const kv_namespaces = [null, { binding: "VINEXT_KV_CACHE", id: "abc" }];
kv_namespaces.find(ns =>
  (ns && typeof ns === "object" && ns.binding === "VINEXT_KV_CACHE") ||
  ns.binding === "VINEXT_CACHE");
// → CRASH: Cannot read properties of null (reading 'binding')

A null or non-object entry in kv_namespaces is valid JSON, so a hand-edited or unusual wrangler.jsonc would crash parseWranglerConfig (and therefore vinext deploy / TPR), rather than skipping the bad entry. oxlint doesn't catch it because ns is typed Record<string, unknown>, but the runtime value can be null.

Fix — restore the guard around both branches:

const vinextKV = config.kv_namespaces.find(
  (ns: Record<string, unknown>) =>
    ns &&
    typeof ns === "object" &&
    (ns.binding === "VINEXT_KV_CACHE" || ns.binding === "VINEXT_CACHE"),
);

The TOML path (tpr.ts:277) is safe because it works off regex match groups, not raw objects.

Also worth a focused test: neither the legacy-VINEXT_CACHE fallback nor the null-entry path is covered by tests/deploy.test.ts (the only KV-extraction test uses a single well-formed VINEXT_KV_CACHE entry at :2651). A two-line test for each would lock in the fallback and prevent this regression from recurring.

2. Stale doc reference still present (minor)

README.md is updated, but .opencode/agents/nextjs-tracker.md:62 still lists the old path:

kv-cache-handler.ts # KV-backed CacheHandler

That file is now cloudflare/cache/kv-data-adapter.runtime.ts (and cloudflare-cdn-cache.tscloudflare/cache/cdn-adapter.runtime.ts). Worth updating so the tracker's source map matches reality.

3. inlineOptions comment/error still overpromises (minor)

cache/cache-adapters-virtual.ts:52-53 and the thrown error claim options that "are not serializable" are rejected, but JSON.stringify only throws on circular refs / BigInt — functions, undefined, and symbols are silently dropped. Since options is documented as JSON-serializable this is acceptable behavior, but the wording implies stronger validation than it provides. Either tighten the wording or note that non-JSON values are dropped rather than rejected.


Everything else looks solid: the config-time-vs-request-time split is clean (builders return inert descriptors via import.meta.resolve + fileURLToPath and never touch the Workers runtime), registration is wired consistently across App Router (app-rsc-handler.ts:796) and Pages Router (pages-server-entry.ts:583/:1047) plus the worker entries with env, the resilient skip-on-failure behavior is correct, and the cloudflare/index.ts/shims/cdn-cache.ts re-export paths are consistent with the new layout. vp check is clean and tests/cache-adapters-config.test.ts (17) + tests/deploy.test.ts (222) pass locally.

Item 1 is the one to fix before merge — it's a straightforward regression in the new fallback code. Items 2–3 are minor.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Jun 4, 2026

Review posted to PR #1733.

Summary

This is a re-review of PR #1733 (feat(cache): configure cache adapters from vite plugin config). I verified that the prior review's findings were addressed, then audited the new code. I read the full diff, reproduced a bug, and ran the relevant suites (cache-adapters-config 17 passed, deploy 222 passed) plus vp check (clean).

Prior findings — status:

New findings I posted:

  1. Blocking — regression in the fallback fix. The VINEXT_CACHE fallback added to parseWranglerConfig (tpr.ts:195-199) dropped the null-guard for its second branch. Due to &&/|| precedence, a null/non-object entry in kv_namespaces now throws TypeError: Cannot read properties of null (reading 'binding'), crashing vinext deploy/TPR instead of skipping the entry. The original main code guarded the whole predicate. I reproduced it and gave the one-line fix (move both binding checks inside the ns && typeof ns === "object" guard). No test covers the fallback or the null path.

  2. Minor — stale doc: .opencode/agents/nextjs-tracker.md:62 still references the renamed kv-cache-handler.ts.

  3. Minor — inlineOptions overpromises: the comment/error claims non-serializable options are rejected, but JSON.stringify silently drops functions/undefined/symbols.

The rest of the feature (config-time vs request-time split, cross-runtime registration wiring, resilient skip-on-failure) looks correct. Item 1 is the one to fix before merge.

github run

@james-elicx
Copy link
Copy Markdown
Member Author

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: feat(cache): configure cache adapters from vite plugin config

Picking up from the two prior reviews. Both blocking findings are now resolved, and I verified the fixes directly:

  • Prior #1 (legacy VINEXT_CACHE fallback) + the regression it introduced — fixed. parseWranglerConfig now matches both VINEXT_KV_CACHE and the legacy VINEXT_CACHE, and the null-guard is correctly restored at tpr.ts:195-200:

    ns && typeof ns === "object" && (ns.binding === "VINEXT_KV_CACHE" || ns.binding === "VINEXT_CACHE")

    I reproduced both paths against the current predicate: a [null, {binding: "VINEXT_CACHE", id: "…"}] array no longer crashes and the legacy entry resolves. The TOML path (tpr.ts:277) was already safe (regex groups). Good.

  • Prior #4 (env-first ordering invariant) — addressed with clear comments at deploy.ts:589 / app-router-entry.ts:71 / app-rsc-handler.ts:792-795. Both Workers entries register with env before the no-env call inside the shared handler. Invariant holds.

  • Prior #2 (stale docs) — fully resolved. No remaining references to kv-cache-handler.ts / cloudflare-cdn-cache.ts in README.md or .opencode/agents/nextjs-tracker.md. The README cache example, subpaths, and VINEXT_KV_CACHE default are all accurate.

vp check is clean. cache-adapters-config (17), cache-adapters-build (1), and deploy (230) all pass locally.

One new finding, plus a couple of carry-over minors.

1. CloudflareCdnCacheAdapter is no longer importable — undocumented breaking change (blocking)

On main, vinext/cloudflare re-exported the class:

// packages/vinext/src/cloudflare/index.ts (main)
export { KVCacheHandler } from "./kv-cache-handler.js";
export { CloudflareCdnCacheAdapter } from "./cloudflare-cdn-cache.js";

This PR drops the CloudflareCdnCacheAdapter re-export — cloudflare/index.ts now exports only KVCacheHandler and runTPR. The class still exists, but it moved into cloudflare/cache/cdn-adapter.runtime.ts, and no public export subpath reaches it:

  • ./cloudflare no longer re-exports it.
  • ./cloudflare/cache/cdn-adapter (the new export) resolves to the builder file, which only exports cdnAdapter() — not the class.
  • The .runtime.ts file that holds the class isn't in package.json#exports.

So anyone using the documented imperative API —

import { CloudflareCdnCacheAdapter } from "vinext/cloudflare";
import { setCdnCacheAdapter } from "vinext/shims/cdn-cache";
setCdnCacheAdapter(new CloudflareCdnCacheAdapter());

— breaks after upgrading. This directly contradicts the PR description's claim that "the imperative setDataCacheHandler() / setCdnCacheAdapter() API is unchanged." KVCacheHandler was preserved (still re-exported from cloudflare/index.ts, and apps/web/worker/index.ts:8 still imports it), so the asymmetry looks accidental rather than intended.

Fix options:

  • Re-add export { CloudflareCdnCacheAdapter } from "./cache/cdn-adapter.runtime.js"; to cloudflare/index.ts (cheapest, restores parity with KVCacheHandler), or
  • If the intent is to drop the imperative CDN class from the public surface in favor of cdnAdapter(), call it out explicitly as a breaking change in the PR description / changelog with a migration note.

2. No test for the legacy fallback or the null-entry path (minor, follow-up to prior #1)

The null-guard regression is fixed in code but still unguarded by tests. The only KV-extraction test (deploy.test.ts:2717) uses a single well-formed VINEXT_KV_CACHE entry. The prior re-review explicitly suggested locking in (a) the legacy VINEXT_CACHE fallback and (b) a null/non-object kv_namespaces entry. Two ~4-line parseWranglerConfig cases would prevent this exact regression from recurring.

3. inlineOptions comment/error still overpromises (minor)

cache/cache-adapters-virtual.ts:52-53 and the thrown error say options "are not serializable" are rejected, but JSON.stringify only throws on circular refs / BigInt — functions, undefined, and symbols are silently dropped. Since options is documented as JSON-serializable this behavior is acceptable; the wording just implies stronger validation than it provides. Tighten the wording or note that non-JSON values are dropped rather than rejected.


Everything else looks solid. The config-time-vs-request-time split is clean (builders return inert descriptors via import.meta.resolve + fileURLToPath, never touching the Workers runtime), the runtime factory reads env[binding] lazily and throws a clear error that the resilient registration catches and skips, registration is wired consistently across App Router (app-rsc-handler.ts:796) and Pages Router (pages-server-entry.ts:583/:1047) plus the worker entries with env, and the build test proves an absolute-path adapter bundles with no build-time Workers context.

Item 1 is the one to resolve before merge — either restore the re-export or document the breaking change. Items 2–3 are minor.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Jun 4, 2026

Review posted to PR #1733.

Summary

This is a third-round review of PR #1733 (feat(cache): configure cache adapters from vite plugin config). I verified the prior two reviews' blocking findings were fixed, then audited the new state. I read the full diff, reproduced the previously-flagged paths, and ran the relevant suites (cache-adapters-config 17, cache-adapters-build 1, deploy 230) plus vp check (clean).

Prior findings — all resolved:

  • The VINEXT_CACHE legacy fallback and the null-guard regression it introduced are both fixed (tpr.ts:195-200) — verified by reproducing both the null-entry and legacy-binding paths against the current predicate.
  • Env-first ordering invariant is documented across all three entry points.
  • Stale doc references are gone from both README and the tracker.

New finding I posted:

  1. Blocking — CloudflareCdnCacheAdapter is no longer importable. main re-exported it from vinext/cloudflare; this PR drops that re-export, and no public export subpath reaches the class anymore (./cloudflare/cache/cdn-adapter resolves to the builder, not the .runtime file with the class). This breaks the documented imperative setCdnCacheAdapter(new CloudflareCdnCacheAdapter()) usage and contradicts the PR's "imperative API is unchanged" claim. KVCacheHandler was preserved, so the asymmetry looks accidental. Fix: re-add the re-export, or document it as a breaking change.

Carry-over minors: (2) the legacy-fallback and null-entry paths in parseWranglerConfig are fixed in code but still have no test; (3) the inlineOptions comment/error still overpromises about rejecting non-serializable options.

Item 1 is the one to resolve before merge; items 2–3 are minor.

github run

@james-elicx
Copy link
Copy Markdown
Member Author

/bigbonk review for actual issues, not pointless non-issues.

"CloudflareCdnCacheAdapter is no longer importable" is not an issue. People should be using the config.

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(cache): configure cache adapters from vite plugin config

Re-reviewed against the current head (a43c7df). I read the full diff, traced the registration wiring across every router/runtime entry, built the package layout assumptions, and ran the relevant suites (cache-adapters-config 17, cache-adapters-build 1) plus vp check on the changed files (clean).

Prior blocking findings are resolved, verified directly:

  • parseWranglerConfig legacy VINEXT_CACHE fallback + restored null-guard at tpr.ts:195-200 (JSON path) and the regex-safe TOML path at tpr.ts:277-283.
  • Env-first ordering invariant is documented and holds at every Workers chokepoint: app-router-entry.ts:72 and deploy.ts:590 both call registerConfiguredCacheAdapters(env) before the no-env calls inside the shared handlers (app-rsc-handler.ts:796, pages-server-entry.ts:583/:1047).
  • Stale doc references are gone.

I'm deliberately not re-raising the CloudflareCdnCacheAdapter re-export — using the cache config is the intended path, and the auto-detect default still resolves the class directly via shims/cdn-cache.ts, so there's no functional regression in CDN selection.

The design is sound. The config-time/request-time split is clean (builders return inert descriptors via import.meta.resolve + fileURLToPath, never touching the Workers runtime), and unbundle: true in vite.config.ts guarantees *.runtime.js are emitted at the paths the builders resolve to, so the absolute-path-in-generated-module approach holds for the published package.

Two minor items, neither blocking:

1. viteConfigHasCacheAdapter matches the first cache: { block in the file (minor)

extractCacheBlock (deploy.ts:1240) regex-matches the first \bcache\s*:\s*\{ occurrence. If an unrelated cache: { appears earlier in vite.config.ts (another plugin's option, a nested build config, etc.), the deploy-time ISR-without-adapter guard inspects the wrong block — it could either throw formatMissingCacheAdapterError on a correctly-configured app (false negative) or pass on a misconfigured one (false positive via an unrelated data:/cdn: key). cacheDir is safe (no : immediately after), and the lenient-on-unreadable stance caps the worst case, but the "first match wins" heuristic is the fragile part. Anchoring to a vinext( call site, or just documenting the limitation, would harden it. Low severity given how rare a competing cache: { is in practice.

2. inlineOptions comment/error still overpromises (minor, carry-over)

cache/cache-adapters-virtual.ts:52-60 claims non-serializable options are "rejected", but JSON.stringify only throws on circular refs / BigInt — functions, undefined, and symbols are silently dropped. Since options is documented as JSON-serializable this is acceptable behavior; the wording just implies stronger validation than it delivers. Worth a one-word tweak ("dropped" vs "rejected") or leave as-is.

Everything else — cross-environment registration, resilient skip-on-failure, the tpr.ts fallback, the build test proving absolute-path adapters bundle without a build-time Workers context — looks correct. Nothing here blocks merge.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Jun 4, 2026

Review posted to PR #1733.

Summary

Fourth-round review of PR #1733 (feat(cache): configure cache adapters from vite plugin config). I verified the prior blocking findings are resolved, traced the registration wiring across all router/runtime entries, and ran the relevant suites (cache-adapters-config 17, cache-adapters-build 1) + vp check on the changed files — all clean.

Prior blocking items — all resolved (verified directly):

  • parseWranglerConfig legacy VINEXT_CACHE fallback + restored null-guard (tpr.ts:195-200 JSON, :277-283 TOML).
  • Env-first ordering invariant holds at every Workers chokepoint (app-router-entry.ts:72, deploy.ts:590 register with env before the no-env handler calls).
  • Stale docs gone.

Per your instruction, I did NOT re-raise CloudflareCdnCacheAdapter importability — the cache config is the intended path, and I confirmed the CDN auto-detect default still resolves the class directly via shims/cdn-cache.ts, so there's no functional regression.

New/carry-over minors (neither blocking):

  1. viteConfigHasCacheAdapter (deploy.ts:1240) matches the first cache: { block in the file — an unrelated earlier cache: { could mislead the deploy-time ISR guard (false negative/positive). Lenient design caps the worst case; low severity.
  2. inlineOptions comment/error still says non-serializable options are "rejected" when JSON.stringify silently drops functions/undefined/symbols (carry-over).

The design is sound — clean config-time/request-time split, unbundle: true guarantees the *.runtime.js files land where the builders resolve them, resilient skip-on-failure is correct. Nothing blocks merge.

github run

Comment thread apps/web/worker/index.ts
Comment on lines 36 to 42
let cacheHandlerInstalled = false;
function installCacheHandler(env: Env): void {
if (cacheHandlerInstalled) return;
setCacheHandler(new KVCacheHandler(env.VINEXT_CACHE));
setCacheHandler(new KVCacheHandler(env.VINEXT_KV_CACHE));
cacheHandlerInstalled = true;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this - add in config instead....

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/bigbonk address this comment

Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Jun 4, 2026

I've addressed the comment. Here's a summary of the change.

Summary

The review comment on apps/web/worker/index.ts:42 asked to remove the imperative cache handler installation and configure it via the plugin config instead — exactly the pattern this PR introduces.

Changes:

  1. apps/web/vite.config.ts — added the cache config to vinext(), using kvDataAdapter() (which defaults to the VINEXT_KV_CACHE binding, matching wrangler.jsonc):

    vinext({
      cache: {
        data: kvDataAdapter(),
      },
    })
  2. apps/web/worker/index.ts — removed:

    • The KVCacheHandler / setCacheHandler imports
    • The installCacheHandler() function and cacheHandlerInstalled guard
    • The installCacheHandler(env) call in fetch

The KV-backed CacheHandler is now registered automatically per request via the config-driven virtual:vinext-cache-adapters module, which resolves the VINEXT_KV_CACHE binding from env at request time. The worker entry is now image-optimization-only, delegating everything else to vinext.

vp check passes clean on both changed files.

github run

@james-elicx james-elicx enabled auto-merge (squash) June 4, 2026 23:26
@james-elicx james-elicx merged commit f0f6aa7 into main Jun 4, 2026
39 checks passed
@james-elicx james-elicx deleted the claude/sweet-germain-225293 branch June 4, 2026 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant