feat(cache): extract Cloudflare cache adapters into @vinext/cloudflare - #1748
Conversation
commit: |
|
| "private": true, | ||
| "scripts": { | ||
| "build": "vp run vinext#build", | ||
| "build": "vp run @vinext/cloudflare#build && vp run vinext#build", |
There was a problem hiding this comment.
cant we use vp task runner to run this in all packages?
There was a problem hiding this comment.
I tried, but the task runner can't order this one. vinext depends on @vinext/cloudflare, and @vinext/cloudflare has vinext as a peer dep, so the workspace graph is a cycle. Every graph-ordered variant fails:
$ vp run --filter "vinext..." build
error: Cycle dependency detected: vinext#build -> @vinext/cloudflare#build -> vinext#build
-r, -F vinext -F @vinext/cloudflare, and --ignore-depends-on all hit the same cycle (vp counts peerDependencies in the graph). The build order is actually irrelevant here — both packages compile from source via tsconfig paths, so neither needs the other's dist first — so I kept the two explicit invocations. Open to a better idea if vp has a 'select these packages, don't topo-order' mode I missed.
|
/bigbonk review |
james-elicx
changed the title
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
…flare Move the Cloudflare KV data cache and edge CDN cache adapters out of vinext into a new publishable @vinext/cloudflare package: - cache/kv-data-adapter(.runtime).ts (KVCacheHandler, kvDataAdapter) - cache/cdn-adapter(.runtime).ts (CloudflareCdnCacheAdapter, cdnAdapter) tpr.ts stays in vinext. vinext now depends on @vinext/cloudflare (workspace:*) and the package declares vinext as a peer dep; both build from source via tsconfig paths so there is no build-order cycle. The vinext/cloudflare barrel still re-exports KVCacheHandler for back-compat. Wires up tsconfig paths, a vitest source alias, root build/postinstall, and the preview/publish workflows for the new package. Updates internal consumers (apps/web, examples/workers-cache), docs, and tests.
vinext now depends on @vinext/cloudflare, which isn't published to npm yet. The create-next-app smoke test packs vinext locally and resolves its deps from the registry, so the install (and dev server) failed with ERR_PNPM_FETCH_404 for @vinext/cloudflare. Pack @vinext/cloudflare alongside vinext and add a pnpm override in the scaffolded project pointing at the local tarball so the dependency resolves offline.
- Remove the root barrel export from @vinext/cloudflare; expose only the ./cache/* subpaths via a wildcard export (no root main/types). - vinext/cloudflare re-exports KVCacheHandler from the full subpath. - Drop the redundant .npmignore (the package.json "files" allowlist already restricts the publish to dist). - Remove the unsupported imperative setCacheHandler/KVCacheHandler usage from both READMEs; the cache plugin config is the supported approach. - Simplify test wiring: drop the now-unused @vinext/cloudflare tsconfig path and dedupe the vitest source alias into a shared constant.
The @vinext/cloudflare config uses vite-plus and nothing imports vite, so the vite devDependency was unused. build/check/knip stay green without it.
8473ca4 to
9d9c09a
Compare
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: extract Cloudflare cache adapters into @vinext/cloudflare
I checked out the branch, ran the full verification (vp check, vp exec knip, and the targeted test files), simulated the published-package exports resolution, and traced the runtime/build wiring. The change is correct and ships cleanly — all local checks are green and the packaging resolves end-to-end.
Verified working
vp check— clean (format + lint + 755 files type-checked).vp exec knip— clean. TheentriesFromPackageJson("packages/cloudflare/package.json")entry derivessrc/cache/*.{ts,tsx}from the./cache/*wildcard, which globs all four files (incl. the.runtime.tsones), so they aren't flagged as unused.- Targeted tests pass:
kv-cache-handler,cloudflare-cdn-cache,cache-adapters-config,shims,tpr-kv-keys,deploy,app-route-handler-response,isr-cache,fetch-cache. - Exports resolution —
@vinext/cloudflare/cache/cdn-adapter.runtime→./dist/cache/cdn-adapter.runtime.js(+ matching.d.ts), and all four dist files exist after build. The./cache/*wildcard correctly covers the.runtimesubpaths consumers import. - Build externalization —
skipNodeModulesBundle: truekeeps thevinext↔@vinext/cloudflareedge external in both dists. The builtcloudflareruntime keepsvinext/shims/*as external imports (resolved via thevinextpeer dep + vinext's./shims/*wildcard export). vinext's builtcloudflare/index.jscorrectly re-exportsKVCacheHandlerfrom@vinext/cloudflare/cache/kv-data-adapter.runtime, preserving thevinext/cloudflarebarrel for backward compat. - Circular runtime edge is real (
cdn-adapter.runtimeimports thegetRequestExecutionContextvalue fromvinext/shims/request-context, andshims/cdn-cacheimportsCloudflareCdnCacheAdapterfrom@vinext/cloudflare). ESM tolerates it because both bindings are only used lazily (class/fn definitions, not top-level execution), and theinstanceof CloudflareCdnCacheAdaptercheck stays single-instance via the source alias in tests and the single dist file in prod. Confirmed by the passing cdn-cache resolver tests. - No lingering
vinext/cloudflare/cache/*references anywhere in source/docs/fixtures.
Minor observations (non-blocking)
-
PR description says
publish.ymlbumps + publishes the new package — that file doesn't exist. Releases run through changesets (release.yml). The actual mechanism is fine:discoverPublishablePackages()inscripts/create-changeset.mtsauto-discovers any non-privatepackages/*, andchangeset publishwill publish@vinext/cloudflare@0.0.5on the next release run because that version isn't on npm yet (independent of whether a changeset bumps it). Worth fixing the PR body to avoid future confusion, but no code change needed. -
First-publish ordering of the bidirectional edge. Published
vinextwill pin@vinext/cloudflareat its convertedworkspace:*version (currently0.0.5).changeset publishpublishes in dependency order, so@vinext/cloudflarelands beforevinextreferences it — but it's worth a one-time sanity check on the first real release that the npm consumer ofvinextresolves@vinext/cloudflarewithout a registry gap (the CIcreate-next-appjob already exercises this offline via the local-tarball override, which is a good guard). -
Duplicated
cache-control-metadatahelpers.packages/cloudflare/src/utils/cache-control-metadata.tsre-implementsisUnknownRecord/readCacheControlNumberField(and a privatereadRecordField) that already live in vinext core. This is a deliberate self-containment trade-off (avoids importing an unexported deep vinext internal), and the JSDoc says so. Fine as-is; just note the two copies can drift — if the cache-control shape ever changes, both need updating.
LGTM. The packaging, build externalization, exports map, and backward-compat barrel all check out.
|
Review posted to PR #1748. Summary of my findings: Verdict: LGTM — ships cleanlyI checked out the branch, ran the full local verification, and traced the packaging/runtime/build wiring end-to-end. Verified working:
Minor (non-blocking) notes:
No code changes required. |
1 participant
What
Moves the Cloudflare cache adapters out of
vinextinto a new publishable@vinext/cloudflarepackage (packages/cloudflare).Moved (history preserved via rename):
cache/kv-data-adapter.ts+.runtime.ts—KVCacheHandler,kvDataAdapter,ENTRY_PREFIXcache/cdn-adapter.ts+.runtime.ts—CloudflareCdnCacheAdapter,cdnAdaptertpr.tsstays in vinext (only the cache adapters moved, per scope).Dependency model
vinext now depends on
@vinext/cloudflare(workspace:*); the new package declaresvinextas a peer dependency. This is the intentional (approved) bidirectional edge:shims/cdn-cache.tsimportsCloudflareCdnCacheAdapterfrom@vinext/cloudflare/cache/cdn-adapter.runtimecloudflare/tpr.tsimportsENTRY_PREFIXfrom therevinext/cloudflarebarrel still re-exportsKVCacheHandlerfor backward compatibilityNeither package needs the other's
distto build — type imports resolve to source via tsconfigpaths, so there's no build-order deadlock.Wiring
@vinext/cloudflare+@vinext/cloudflare/cache/*path mappings to source@vinext/cloudflare/cache→ source alias in both test projects (mirrors the existingvinext/shimsalias) so tests run against source without a prior buildbuild/postinstallbuild@vinext/cloudflare#buildbeforevinext#buildpreview-release.yml(pkg-pr-new) publishes both packages;publish.ymlbumps + publishes@vinext/cloudflarein lockstep with vinextapps/web,examples/workers-cache, READMEs, thedeploy.tserror message, and thecacheplugin-option JSDoc now reference@vinext/cloudflare/cache/*Breaking change
External users importing
vinext/cloudflare/cache/*must switch to@vinext/cloudflare/cache/*. Thevinext/cloudflarebarrel still re-exportsKVCacheHandlerfor compatibility.Verification
vp check— clean (format + lint + types)pnpm knip— cleankv-cache-handler,cloudflare-cdn-cache,cache-adapters-config,app-route-handler-response,deploy,tpr-kv-keys,shims,isr-cache,fetch-cache,app-routerLetting CI run the full suite + Playwright E2E.