fix(build): bundle @vinext/cloudflare into vinext to break dependency cycle - #1797
Conversation
… cycle vinext consumed a few runtime helpers (KVCacheHandler, CloudflareCdnCacheAdapter, ENTRY_PREFIX) from @vinext/cloudflare via a runtime `dependencies` edge. Combined with @vinext/cloudflare's `peerDependencies` on vinext, this formed a cycle that forced changesets to force-major @vinext/cloudflare on every vinext release (mitigated defensively in #1764). Bundle the small amount of @vinext/cloudflare code vinext actually imports into vinext's dist via tsdown's deps.neverBundle, and move @vinext/cloudflare to devDependencies. The published install graph now points one way (@vinext/cloudflare -> vinext). @vinext/cloudflare remains a published package: user vite.config files still import cdnAdapter()/kvDataAdapter(), and the generated worker resolves its *.runtime.js factories by absolute path. Its bundled-in vinext/shims/* imports stay external and resolve via vinext's own package exports (Node self-reference). Drop the now-obsolete create-next-app CI steps that packed and overrode @vinext/cloudflare (vinext's tarball no longer declares it as a dependency).
commit: |
|
Use a single intent-revealing neverBundle predicate with a BUNDLED_DEPS carve-out and document why the carve-out must live in neverBundle (tsdown rejects skipNodeModulesBundle + alwaysBundle as mutually exclusive, and a catch-all neverBundle takes precedence over alwaysBundle).
…predicate The previous approach replaced deps.skipNodeModulesBundle with a custom neverBundle predicate. That broke tsdown's rewriting of vinext's own `vinext/shims/*` tsconfig-path self-imports to relative paths, leaving them as bare `vinext/*` specifiers in 46 files. Across Vite's separate RSC/SSR/client dev module graphs those bare self-references resolved to distinct module instances, breaking identity checks (e.g. `instanceof ReadonlyURLSearchParams`). Keep skipNodeModulesBundle: true untouched and instead bundle @vinext/cloudflare by aliasing its cache/* subpath to source. skipNodeModulesBundle externalizes bare package specifiers before tsconfig paths apply, so the alias rewrites the import to a file path up front; tsdown then treats it as local source and bundles it. The bundled code's own vinext/shims/* imports still resolve to vinext's relative output (single module instance). Output is byte-identical to the pre-change build except the added bundled cloudflare files and one cosmetic import reorder.
|
/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: fix(build): bundle @vinext/cloudflare to break dependency cycle
I built vinext with these changes, inspected the dist output, ran the affected test files (399 passing), and traced the resulting dependency/module graph. This is correct and ready to merge. Details below.
Verified ✅
Cycle broken. @vinext/cloudflare is moved dependencies → devDependencies (and dropped from the lockfile importer's deps). The published install graph now points one way: @vinext/cloudflare → vinext.
Bundling actually happened. After vp run vinext#build, the consuming files import the bundled local copies — no external @vinext/cloudflare runtime imports remain:
dist/cloudflare/tpr.js→import { ENTRY_PREFIX } from "./src/cache/kv-data-adapter.runtime.js"dist/cloudflare/index.js→import { KVCacheHandler } from "./src/cache/kv-data-adapter.runtime.js"dist/shims/cdn-cache.js→import { CloudflareCdnCacheAdapter } from "../cloudflare/src/cache/cdn-adapter.runtime.js"
The only remaining @vinext/cloudflare strings in dist are JSDoc comments and the user-facing "configure a cache adapter" error message in deploy.js — both intentional.
Transitive deps came along. cdn-adapter.runtime / kv-data-adapter.runtime import ../utils/cache-control-metadata, and that got bundled too (dist/cloudflare/src/utils/cache-control-metadata.js), as expected once the alias turns the subpath into local source.
No duplicate shim modules (the hazard called out in the config comment). The bundled cloudflare runtime's vinext/shims/request-context import was rewritten to a relative path resolving to the single shared dist/shims/request-context.js (confirmed exactly one request-context.js in dist). So the instanceof / single-module-instance concern is satisfied.
CI cleanup is sound. The removed "Pack @vinext/cloudflare" + "Override @vinext/cloudflare to the local tarball" steps existed only because vinext's tarball declared a dependency on the unpublished @vinext/cloudflare. With that dependency gone, the scaffolded create-next-app no longer needs the cloudflare tarball to install. Nothing else in the create-next-app job references it.
Published consumers unaffected. @vinext/cloudflare keeps its ./cache/* export and peerDependencies: { vinext }; user vite.config imports of cdnAdapter()/kvDataAdapter() and the generated worker's absolute-path *.runtime.js resolution are unchanged. The supported pattern already requires users who configure caching to declare @vinext/cloudflare directly (examples/workers-cache).
Tests + checks. tests/deploy, tests/cache-adapters-config, tests/kv-cache-handler, tests/build-optimization → 399 passed. vp check packages/vinext/vite.config.ts clean (format + lint + types).
Minor notes (non-blocking)
-
Alias is a prefix match.
"@vinext/cloudflare/cache" → <dir>rewrites@vinext/cloudflare/cache/foo→<dir>/fooby substring prefix. It's safe today because@vinext/cloudflareonly exposes./cache/*, but a hypothetical future sibling subpath like@vinext/cloudflare/cache-controlwould be misrewritten by the same prefix. Not a problem now; just worth a mental note if more subpaths are added. -
Tight coupling to
@vinext/cloudflare's internal source layout. Bundling from../cloudflare/src/cachemeans vinext's build now depends on cloudflare's internal file structure (*.runtime.ts,utils/cache-control-metadata.ts), not just its public./cache/*export. If cloudflare reorganizes those files, vinext's build breaks. The affected tests (deploy,cache-adapters-config) should catch that, so this is acceptable — but a one-line comment incloudflare/src/cachenoting "these paths are bundled into vinext; see vinext/vite.config.ts" would help future maintainers.
Nice, well-documented change — the config comments explaining why skipNodeModulesBundle must stay and how the alias interacts with it are exactly the kind of context that prevents future regressions.
|
Review posted to PR #1797. SummaryI reviewed PR #1797 by building vinext with the changes, inspecting the dist output, running the affected tests (399 passing), and tracing the dependency/module graph. The PR is correct and ready to merge. Verified:
Minor non-blocking notes I raised:
No correctness issues found. |
1 participant
Problem
vinextand@vinext/cloudflareform a circular dependency:vinextonly needs a handful of runtime helpers from@vinext/cloudflare:cloudflare/tpr.ts→ENTRY_PREFIXcloudflare/index.ts→ re-exportsKVCacheHandlershims/cdn-cache.ts→CloudflareCdnCacheAdapter(auto-detect edge default)Combined with
@vinext/cloudflare'speerDependenciesonvinext, this cycle forced changesets to force-major@vinext/cloudflareon essentially everyvinextrelease (mitigated defensively in #1764, but the cycle itself remained).Fix
Bundle the small amount of
@vinext/cloudflarecodevinextactually imports intovinext'sdist(via tsdown'sdeps.neverBundle), and move@vinext/cloudflaretodevDependencies. The published install graph now points one way:@vinext/cloudflare → vinext.@vinext/cloudflareremains a published package — nothing user-facing changes:vite.configfiles still importcdnAdapter()/kvDataAdapter()from@vinext/cloudflare/cache/*.*.runtime.jsfactories by absolute path from@vinext/cloudflare.vinext/shims/*imports stay external and resolve throughvinext's own package exports at runtime (Node self-referencing) — identical to how@vinext/cloudflareresolves them today.Also drops the now-obsolete
create-next-appCI steps that packed and overrode@vinext/cloudflare, sincevinext's tarball no longer declares it as a dependency.Verification
vp run vinext#build→disthas no@vinext/cloudflareruntime imports; internal imports now point at bundled local files (e.g../src/cache/kv-data-adapter.runtime.js). Generated.d.tslikewise reference local paths (remaining@vinext/cloudflarestrings are JSDoc only).vp run @vinext/cloudflare#build→ still builds independently.deploy,cache-adapters-config,kv-cache-handler,isr-cache,build-optimizationall green (463 tests).vp checkon the config +pnpm knipclean.Notes
@vinext/cloudflaredirectly (seeexamples/workers-cache). No scaffolding relied on transitive resolution.devDependencies.