Repository navigation
feat(cloudflare): move deploy command to cloudflare package - #2405
Conversation
|
/bigbonk review for issues |
commit: |
|
james-elicx
changed the title
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Review: Move deploy command to
|
|
Posted the review to PR #2405. Summary of findings: Review outcome: LGTM, no blocking issuesThis is a clean refactor moving the Cloudflare deploy implementation/CLI into The one risk worth scrutiny — Verified end-to-end:
Two non-blocking notes: |
5f115d0 to
0e90a1c
Compare
|
/bigbonk review for issues |
Review: Move deploy command to
|
|
Posted the review to PR #2405. Review outcome: LGTM, no blocking issuesClean refactor moving the Cloudflare deploy implementation/CLI into Verified end-to-end (rebuilt both packages first — initial
Two non-blocking notes: several |
0e90a1c to
5cc3ee7
Compare
|
/bigbonk review for issues |
Review: Move deploy command to
|
|
Posted the review to PR #2405. Review outcome: one blocking bug found🔴 Blocking — TPR When const prodServerPath = path.resolve(import.meta.dirname, "..", "server", "prod-server.js");
Impact: Suggested fix: resolve via the peer dependency, mirroring how const req = createRequire(path.join(root, "package.json"));
const prodServerPath = req.resolve("vinext/server/prod-server");Two non-blocking notes ( Note: the two prior |
5cc3ee7 to
f1d2316
Compare
|
Fixed the TPR blocker: |
|
/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: Move deploy command to @vinext/cloudflare
Reviewed the full diff at the current HEAD (which includes the TPR fix from the last comment), rebuilt both packages, ran the targeted tests, type-checked all changed source + test files, and exercised both CLIs from dist. The refactor is clean and the previously-reported blocking bug is now fixed. No blocking issues found.
The previously-blocking TPR bug is fixed ✅
packages/cloudflare/src/tpr.ts:642 now resolves prod-server through the app's installed vinext peer instead of a hardcoded relative path:
export function resolveVinextProdServerPath(root: string): string {
return createRequire(path.join(root, "package.json")).resolve("vinext/server/prod-server");
}I verified this against both built outputs:
packages/cloudflare/dist/tpr.js:395keeps thecreateRequire(...).resolve("vinext/server/prod-server")form (resolves via the peer dep).- The bundled-into-vinext copy at
packages/vinext/dist/packages/cloudflare/src/tpr.js:395is identical, sovinext deploy --experimental-tpralso resolves correctly. vinextexports./server/prod-server(packages/vinext/package.json:32), and the new regression test (tests/tpr-kv-keys.test.ts:95) builds a fake app root with avinextpeer and asserts the resolved path — closing the coverage gap that let the bug ship.
Verified
Bundling / dependency graph (the main risk) — vinext/src/cli.ts:31-32 statically imports @vinext/cloudflare/internal/{deploy,deploy-help} while @vinext/cloudflare is only a devDependency of vinext. This is safe because packages/vinext/vite.config.ts:84-87 aliases @vinext/cloudflare/internal and @vinext/cloudflare/cache to local source, so the deploy/tpr/deploy-config code is bundled into vinext's dist. Confirmed:
dist/cli.jshas no residual@vinext/cloudflareimport (only deprecation/help strings).- The deploy code emits at
dist/packages/cloudflare/src/{deploy,tpr,deploy-config,deploy-help}.js, and thevinext/internal/*imports are rewritten to relative paths (../../../server/headers.js, etc.) that all exist indist. node dist/cli.js deploy --help→ exit 0 with the deprecation warning.- Reverse direction:
@vinext/cloudflarebuildsunbundle, sodist/tpr.js/deploy.jskeepvinext/internal/*andvinext/server/prod-serveras bare externals resolved via thevinextpeer dep;node packages/cloudflare/dist/cli.js deploy --help→ exit 0.
Export surface — every vinext/internal/* subpath the relocated cloudflare source imports (server/headers, server/isr-cache, server/app-page-cache, build/run-prerender, config/dotenv, config/next-config, utils/project) is added to packages/vinext/package.json exports, and @vinext/cloudflare adds ./internal/{deploy,deploy-help,deploy-config,tpr} plus the vinext-cloudflare bin. The tpr.ts self-import of @vinext/cloudflare/cache/kv-data-adapter.runtime is covered by the ./cache/* wildcard.
utils/project.ts merge — all prior exports preserved; the moved helpers (detectProject, getMissingDeps, formatMissingCloudflarePluginError, findViteConfigPath, findInNodeModules, hasWranglerConfig, isPackageResolvable, ProjectInfo) are present and consumed correctly by deploy.ts/deploy-config.ts.
Backward compat — vinext/cloudflare still re-exports KVCacheHandler/runTPR (packages/vinext/dist/cloudflare/index.js resolves to bundled local chunks). The vinext deploy deprecated shim warns and delegates.
No stale references to old paths (cloudflare/project, cloudflare/tpr, cloudflare/deploy-config, src/deploy.js) anywhere outside the relocated source.
Checks run
vp test run tests/deploy.test.ts tests/tpr-kv-keys.test.ts→ 271 passed (was 270; +1 from the new prod-server resolution test)vp checkon all changed source + test files → format + lint + types cleanvp run @vinext/cloudflare#buildandvp run vinext#build→ both succeed
Non-blocking notes
parsePositiveIntegerArgduplication — copied byte-for-byte intocloudflare/src/deploy.ts:66and de-exported from vinext'scli-args.ts(no remaining cross-importers). Fine for now, but the two copies can drift. Also noteparseDeployArgsuses the strictparsePositiveIntegerArgonly for--prerender-concurrency, while--tpr-*flags use the looser localparseIntArg(allows non-positive, callsprocess.exit) — pre-existing inconsistency, now duplicated.npx @vinext/cloudflare deployrelies on npx's single-bin fallback since the bin is namedvinext-cloudflare, not matching the scoped package name. Works against the local package; worth a smoke check against the published tarball at release time.- Self-referential bare imports inside
vinext(vinext/internal/utils/project, etc.) only resolve via the vite alias + tsconfig path + package.json export triple. Verified by the build, but consider keeping intra-package imports relative and reserving the bare specifier for genuine cross-package boundaries.
LGTM.
|
Posted the review to PR #2405. Review outcome: LGTM, no blocking issuesI reviewed the full diff at current HEAD (which includes the TPR fix from the last comment), rebuilt both packages, ran targeted tests, type-checked all changed files, and exercised both CLIs from The previously-blocking TPR bug is fixed. Verified end-to-end:
Three non-blocking notes: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: Move deploy command to @vinext/cloudflare
Reviewed at current HEAD (982b0e1, which adds the new fix(cloudflare): expose scoped deploy package bin commit on top of the previously-reviewed state). Rebuilt both packages, ran the targeted tests, and traced bin/import resolution. The TPR bug from the earlier round is fixed and the core refactor is sound, but the new bin-rename commit does not do what it intends and is a minor regression.
🟡 The @vinext/cloudflare bin key does not produce a @vinext/cloudflare executable
packages/cloudflare/package.json:11-13:
"bin": {
"@vinext/cloudflare": "dist/cli.js"
},Commit 982b0e1 changed the bin key from vinext-cloudflare → @vinext/cloudflare to "expose scoped deploy package bin." But npm and pnpm strip the scope from a scoped bin key, so the installed executable is named cloudflare, not @vinext/cloudflare. Verified against real installs of a package with exactly this bin block:
# npm
$ ls node_modules/.bin/
cloudflare
# pnpm
$ ls node_modules/.bin/
cloudflare
Implications:
- The intent isn't achieved. There is no
@vinext/cloudflarebinary on disk.npx @vinext/cloudflare deploy(the form used everywhere in the README) still works — but only via npx's single-bin fallback, exactly as it did with the oldvinext-cloudflarename. So this commit changes nothing about the documented invocation. - It's a net regression on the local-bin name. The resulting
node_modules/.bin/cloudflareis a generic, collision-prone name (anything that wants acloudflarecommand in.binnow conflicts), whereas the priorvinext-cloudflarewas clear and namespaced. The previous review's non-blocking note about the single-bin fallback is unchanged by this commit — both before and after,npx @vinext/cloudflare deployrelies on the same fallback.
Suggested fix: revert the bin key to a real, scope-free command name (vinext-cloudflare, or vinext-deploy). A scoped key can't ever produce a scoped executable, so it only obscures the actual installed bin name. Keep the README's npx @vinext/cloudflare deploy guidance — that resolves the package and runs its single bin regardless of the bin's name.
Previously-blocking TPR bug is fixed ✅
packages/cloudflare/src/tpr.ts:642 now resolves prod-server through the app's vinext peer:
export function resolveVinextProdServerPath(root: string): string {
return createRequire(path.join(root, "package.json")).resolve("vinext/server/prod-server");
}Verified in both build outputs — the standalone packages/cloudflare/dist/tpr.js:395 and the bundled-into-vinext copy at packages/vinext/dist/packages/cloudflare/src/tpr.js:395 both keep the createRequire(...).resolve(...) form. The new regression test (tests/tpr-kv-keys.test.ts:95) builds a fake app root with a vinext peer and asserts the resolved path, closing the coverage gap.
Everything else checks out
- Bundling / dependency graph.
vinext/src/cli.ts:31-32statically imports@vinext/cloudflare/internal/{deploy,deploy-help}while@vinext/cloudflareis only a devDependency of vinext. Safe:packages/vinext/vite.config.ts:85-86aliases@vinext/cloudflare/internal+/cacheto local source, so the deploy/tpr/deploy-config code is bundled into vinext's dist withvinext/internal/*imports rewritten to relative paths (../../../utils/project.js,../../../build/run-prerender.js). Reverse direction externalizesvinext/internal/*andvinext/server/prod-servervia the peer dep.node packages/vinext/dist/cli.js deploy --helpandnode packages/cloudflare/dist/cli.js deploy --helpboth → exit 0. - Export surface — every
vinext/internal/*subpath the cloudflare source imports is added topackages/vinext/package.jsonexports;@vinext/cloudflareadds./internal/{deploy,deploy-help,deploy-config,tpr}. utils/project.tsmerge — all prior exports preserved; the moved helpers (detectProject,getMissingDeps,formatMissingCloudflarePluginError,hasWranglerConfig,isPackageResolvable,findInNodeModules,ProjectInfo) present and consumed bydeploy.ts/deploy-config.ts.- No stale references to old paths (
cloudflare/project,cloudflare/tpr,vinext-cloudflare) anywhere in source/scripts/docs.
Checks run
vp test run tests/deploy.test.ts tests/tpr-kv-keys.test.ts→ 271 passedvp run @vinext/cloudflare#buildandvp run vinext#build→ both succeed
Non-blocking (carried from prior rounds)
parsePositiveIntegerArgduplicated byte-for-byte intocloudflare/src/deploy.ts:66;--prerender-concurrencyuses the strict parser while--tpr-*flags use the looser localparseIntArg— pre-existing inconsistency, now duplicated.- Self-referential bare
vinext/internal/*imports only resolve via the alias + tsconfig path + package.json export triple.
The only thing I'd act on before merge is the bin key — it can't produce the scoped name it's named for, and trades a clear vinext-cloudflare for a generic cloudflare.
|
Review posted to PR #2405. Review outcome: one minor regression foundThis round adds a new commit ( 🟡 The The commit changed the bin key from
Suggested fix: revert to a real scope-free command name ( Verified everything else:
The bin key is the one thing worth acting on before merge. |
982b0e1 to
49989fb
Compare
|
Fixed the bin regression from the last review: /bigbonk review for issues |
There was a problem hiding this comment.
Review: bin regression fix + deploy command rename
Reviewed the new fix(cloudflare): expose deploy cli bin commit (49989fb) at current HEAD. Rebuilt @vinext/cloudflare, ran the targeted tests, type-checked all changed files, and empirically verified bin resolution across npm/pnpm/vpx/vp exec. The previously-flagged bin regression is resolved. No blocking issues.
The bin regression is fixed ✅
packages/cloudflare/package.json:11-14 now ships both keys:
"bin": {
"@vinext/cloudflare": "dist/cli.js",
"vinext-cloudflare": "dist/cli.js"
}I replicated the exact bin block in a throwaway package and installed it with both npm and pnpm. As the prior review noted, the scoped key @vinext/cloudflare is installed scope-stripped as a cloudflare binary — but because the vinext-cloudflare key is also present, the package-runner direction and the local-bin direction both work:
installed .bin/: cloudflare, vinext-cloudflare
vpx @vinext/cloudflare deploy → runs (resolves by package name)
vpx vinext-cloudflare deploy → runs
vp exec vinext-cloudflare deploy → runs (resolves the local bin)
So every documented invocation in this PR is accurate:
npx @vinext/cloudflare deploy— resolves by package name (npx single-bin/name fallback)vpx @vinext/cloudflare deploy— same, verified against the realvpxvp exec vinext-cloudflare deploy— runs thevinext-cloudflarelocal bin
vpx (Execute a command from a local or remote npm package) is the correct Vite+ analog of npx, and vp exec (Execute a command from local node_modules/.bin) is the correct analog for the locally-installed bin. The README/help wording matches the tools' actual semantics.
Verified
- Built CLI —
node packages/cloudflare/dist/cli.js --version→vinext-cloudflare v0.1.2;--helpprints the newvinext-cloudflarename plus the Vite+ block. All help/usage strings consistently renamed from@vinext/cloudflare→vinext-cloudflare. - tpr-demo dependency wiring —
examples/tpr-demo/package.jsonadds@vinext/cloudflare: workspace:*as a dependency (required for thevinext-cloudflarebin to land innode_modules/.binfor the"deploy": "vinext-cloudflare deploy"script). Lockfile change is the single matchinglink:../../packages/cloudflareentry;pnpm install --frozen-lockfilepasses. - CI unaffected —
create-next-app-cloudflare-deploy.ymlandci.ymldeploy viavp exec wrangler deploy, not the renamed bin, so the rename doesn't touch them. - Tests —
vp test run tests/deploy.test.ts tests/tpr-kv-keys.test.ts→ 271 passed;vp checkon all changed source files → format + lint + types clean;vp run @vinext/cloudflare#buildsucceeds. - Doc updates across README,
apps/web, tpr-demo pages/scripts, andvinext deploydeprecation text are consistent and point at valid commands.
Non-blocking
- Deprecation warning moved to stdout —
packages/vinext/src/cli.ts:807changedprintDeployDeprecationWarningfromconsole.warn→console.log, so thevinext deploydeprecation notice now prints to stdout instead of stderr. Deprecation/diagnostic notices conventionally go to stderr so they don't pollute machine-parseable stdout when deploy output is piped. No test asserts the stream. Minor; consider keeping it onconsole.warn. - Scope-stripped
cloudflarebin — the@vinext/cloudflarebin key installs a genericcloudflareexecutable into.bin(collision-prone with anything else wanting acloudflarecommand). It's harmless givenvinext-cloudflareis the documented local name andvpx @vinext/cloudflareresolves by package name regardless, but the scoped key only ever yieldscloudflare, never@vinext/cloudflare. If the goal is solely to supportvpx @vinext/cloudflare, that already works via package-name resolution without the scoped bin key — the key is essentially redundant. - Carried from prior rounds:
parsePositiveIntegerArgduplication incloudflare/src/deploy.ts; self-referential barevinext/internal/*imports resolve only via the alias/tsconfig/export triple.
LGTM.
|
Review posted to PR #2405. Review outcome: LGTM, no blocking issuesThe previously-flagged bin regression is resolved. The PR now ships both bin keys: "bin": {
"@vinext/cloudflare": "dist/cli.js",
"vinext-cloudflare": "dist/cli.js"
}Empirically verified (replicated the exact bin block, installed with npm + pnpm, ran the real
Two non-blocking notes worth acting on:
|
49989fb to
97f5087
Compare
|
Adjusted public docs and website examples back to /bigbonk review for issues |
97f5087 to
4c45345
Compare
There was a problem hiding this comment.
Review: deploy command move + doc adjustments
Reviewed the current HEAD (4c45345, the force-pushed update that adjusts the public docs/website examples back to npx @vinext/cloudflare deploy while keeping vp exec vinext-cloudflare deploy as the Vite+ local-bin form). Rebuilt both packages, ran the targeted tests, type-checked the changed files, ran a frozen-lockfile install, and exercised the bins end-to-end.
🟡 Stale (and now incorrect) docs in the migrate-to-vinext skill
The PR's stated goal is to fix public docs/website examples for the renamed/relocated deploy command, but the migrate-to-vinext skill — which is public, user-facing documentation — was not updated. It still references the deprecated vinext deploy command, and worse, it documents behavior the command no longer has:
-
.agents/skills/migrate-to-vinext/SKILL.md:131:"If the user wants to deploy to Cloudflare Workers, use
vinext deploy. It auto-generateswrangler.jsonc, worker entry, and Vite config if missing, installs@cloudflare/vite-pluginandwrangler, then builds and deploys."This is now factually wrong. The relocated deploy command no longer scaffolds anything — it validates and errors out telling the user to run
vinext init --platform=cloudflarefirst (packages/cloudflare/src/deploy.ts:6"Validates the project was prepared byvinext init --platform=cloudflare";:341/:347throwMissing ... Run \vinext init --platform=cloudflare` first.). I confirmed there is nowriteFile/mkdir/scaffold path left indeploy.ts`. -
Other stale
vinext deployreferences in the same skill:SKILL.md:34(CLI table),SKILL.md:184,SKILL.md:215,.agents/skills/migrate-to-vinext/references/config-examples.md:78,:182,:191.
Since this PR is the one renaming the command (@vinext/cloudflare deploy / vinext-cloudflare deploy) and changing its behavior from generate→validate, the skill should be updated in the same change to avoid shipping a doc that tells migrating users a removed behavior still works. At minimum, SKILL.md:131 should be corrected (deploy validates, it does not generate — users should run vinext init --platform=cloudflare to scaffold), and the command name updated.
Verified (everything else checks out)
Website / README docs — README, packages/cloudflare/README.md, apps/web/app/page.tsx, and the tpr-demo pages/scripts consistently use npx @vinext/cloudflare deploy as the primary form with vp exec vinext-cloudflare deploy documented as the Vite+ local-bin form. The vinext --help Commands list correctly drops the deprecated deploy entry while the shim still functions (warns + delegates) — verified node dist/cli.js deploy --help still works.
Bins — packages/cloudflare/package.json:11-14 ships both @vinext/cloudflare (installs scope-stripped as cloudflare) and vinext-cloudflare keys. After vp install --frozen-lockfile, examples/tpr-demo/node_modules/.bin/ contains both cloudflare and vinext-cloudflare, and .bin/vinext-cloudflare --version → vinext-cloudflare v0.1.2, so tpr-demo's "deploy": "vinext-cloudflare deploy" script resolves. The tpr-demo correctly moved @vinext/cloudflare to dependencies (needed for the local bin); lockfile + frozen install pass.
CLI output — vinext-cloudflare v0.1.2; deploy --help consistently branded vinext-cloudflare, with the runner forms (npx @vinext/cloudflare, vpx @vinext/cloudflare, vp exec vinext-cloudflare) shown in the examples block.
Build / tests — vp run @vinext/cloudflare#build and vp run vinext#build both succeed. After rebuilding vinext, node packages/cloudflare/dist/cli.js deploy --help → exit 0 (the standalone cloudflare CLI requires a fresh vinext/dist for its vinext/internal/* peer-externalized imports — expected). vp test run tests/deploy.test.ts tests/tpr-kv-keys.test.ts → 271 passed; vp check on changed files clean.
Non-blocking (carried from prior rounds)
- Deprecation warning on stdout —
packages/vinext/src/cli.ts:808usesconsole.log(stdout) rather thanconsole.warn(stderr). Deprecation notices conventionally go to stderr so they don't pollute piped deploy output. Minor. - Scope-stripped
cloudflarebin — the@vinext/cloudflarebin key only ever yields a genericcloudflareexecutable (collision-prone). Sincevpx @vinext/cloudflareresolves by package name regardless andvinext-cloudflareis the documented local bin, the scoped key is effectively redundant. parsePositiveIntegerArgduplicated intocloudflare/src/deploy.ts; self-referential barevinext/internal/*imports resolve only via the alias/tsconfig/export triple.
The doc-parity issue in the skill is the one thing worth fixing in this PR, since it's the PR that makes those docs wrong.
|
Review posted to PR #2405. Review outcome: one doc-parity issue worth fixingThe doc-adjustment update is sound — README, 🟡 The one finding: the
Non-blocking notes carried from prior rounds: deprecation warning moved to stdout (conventionally stderr), the scoped |
Summary
@vinext/cloudflarevinext deployas a deprecated compatibility shim that warns and delegatesvinext/utils/project@vinext/cloudflare/internal/*Validation
vp check vite.config.ts packages/cloudflare/package.json packages/vinext/package.json packages/vinext/vite.config.ts packages/vinext/src/cli.ts packages/vinext/src/index.ts packages/vinext/src/init.ts packages/vinext/src/init-cloudflare.ts packages/vinext/src/cloudflare/index.ts packages/vinext/src/utils/project.ts tsconfig.json packages/cloudflare/src/cli.ts packages/cloudflare/src/deploy.ts packages/cloudflare/src/deploy-config.ts packages/cloudflare/src/tpr.ts tests/deploy.test.ts tests/tpr-kv-keys.test.tsvp test run tests/deploy.test.ts tests/tpr-kv-keys.test.tsvp run @vinext/cloudflare#buildvp run vinext#buildnode packages/cloudflare/dist/cli.js deploy --helpnode packages/vinext/dist/cli.js deploy --helpnpx --yes ./packages/cloudflare deploy --helpNote:
vp run vinext#buildstill emits the existing virtual-module externalization warnings, but exits successfully.