Repository navigation
fix(cloudflare): pass the Preview name with --preview-name like cf - #4572
james-elicx wants to merge 1 commit into
Conversation
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 7 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
james-elicx
changed the title
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
7df47e46.
This review, of 7e024b87
💬 Approved with comments — The implementation is correct and well tested. One WARNING: the pinned cf version and the open "*" peer range still allow cf versions that reject the new --preview-name flag.
Findings: 1 warning (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #4572.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] The pinned cf (`1.0.0-beta.1`) and the open `cf: "*"` peer range still allow cf versions that reject `--preview-name`, so named Preview deploys fail with a raw yargs error.
- **File:** packages/cloudflare/src/deploy.ts:726
📋 Code Quality: ⚠️ 1 warning
The change is correct and well tested. The cf version pin and peer range don't yet require a cf release that accepts --preview-name.
Recommendation: 📝 Minor Comments
- [WARNING] The pinned cf (
1.0.0-beta.1) and the opencf: "*"peer range still allow cf versions that reject--preview-name, so named Preview deploys fail with a raw yargs error. (packages/cloudflare/src/deploy.ts:726)
View full review
The parser, validation, CLI wiring and tests are consistent:
parsePreviewsDeployArgsdeclarespreview-nameas a string option and rejects every positional argument with a clear message.- A missing value is still rejected by strict
parseArgs. - The empty-name and null-byte checks are kept.
One issue remains. buildCfDeployArgs now always sends --preview-name when a name is given, but no version requirement stops it from running against a cf that rejects the flag:
- The workspace catalog pins
cf: 1.0.0-beta.1(pnpm-workspace.yaml:117). - The package's
cfpeer range is"*".
✅ All other checks passed
📝 Documentation: ✅ LGTM
The Worker Preview docs and the CLI help text consistently describe --preview-name <name>. They also say that leaving it out makes cf use the CI or Git branch name.
Reviewed commit: 7e024b87
| const isPreviewDeploy = options.target?.type === "preview"; | ||
| const args = isPreviewDeploy ? ["previews", "deploy"] : ["deploy"]; | ||
| if (isPreviewDeploy && options.target?.name) args.push(options.target.name); | ||
| if (isPreviewDeploy && options.target?.name) args.push("--preview-name", options.target.name); |
There was a problem hiding this comment.
The pinned cf (1.0.0-beta.1) and the open cf: "*" peer range still allow cf versions that reject --preview-name, so named Preview deploys fail with a raw yargs error.
cf is yargs-strict, so cf versions before cloudflare/cf#519 fail on this flag with Unknown argument: preview-name. That includes the catalog pin cf: 1.0.0-beta.1 (pnpm-workspace.yaml:117). Today that breaks the Worker Preview deploy in CI. After release, it breaks previews deploy --preview-name for any user on an older cf, because the cf peer range in packages/cloudflare/package.json is "*". Before merging, bump the cloudflare-vite-v2 catalog to the cf release that includes #519, and raise the peer range minimum to that release so incompatible installs are caught at install time.
cf previews deploy replaced its positional Preview name with --preview-name (cloudflare/cf#519). Mirror that in vinext-cloudflare previews deploy and pass the name to cf with the flag.
7e024b8 to
7df47e4
Compare
There was a problem hiding this comment.
Superseded by the review of
3cfc8fef.
This review, of 7df47e46
💬 Approved with comments — The --preview-name change and the empty SHA-named changeset are correct. One WARNING is still open: the pinned cf and the open peer range still allow cf versions that reject the new flag.
Findings: 1 warning
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #4572.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] The pinned cf (`1.0.0-beta.1`) and the open `cf: "*"` peer range still allow cf versions that reject `--preview-name`, so named Preview deploys fail with a raw yargs error.
- **File:** packages/cloudflare/src/deploy.ts:726
📋 Code Quality: ⚠️ 1 warning
Parsing, validation, help text and tests are consistent. The cf version requirement is still open.
Recommendation: 📝 Minor Comments
- [WARNING] The pinned cf (
1.0.0-beta.1) and the opencf: "*"peer range still allow cf versions that reject--preview-name, so named Preview deploys fail with a raw yargs error. (packages/cloudflare/src/deploy.ts:726)
View full review
Since the last review, the only new file is .changeset/2ceb012aaa75a1223ed2b16d511abed939abf1db.md. It is empty, and an empty, SHA-named changeset is the repository's way to drop a commit from the next release, so it does what the description says.
One issue from the last review is still open:
buildCfDeployArgs(packages/cloudflare/src/deploy.ts:726) always sends--preview-namewhen a name is given.- The workspace catalog still pins
cf: 1.0.0-beta.1(pnpm-workspace.yaml:117). - The package's cf peer range is still
"*".
Any cf release from before the flag existed therefore fails a named Preview deploy with a raw argument error.
✅ All other checks passed
📝 Documentation: ✅ LGTM
The Worker Preview docs and the CLI help text both describe --preview-name <name>. Both also say that leaving it out makes cf use the CI or Git branch name.
Reviewed commit: 7df47e46
7df47e4 to
3cfc8fe
Compare
There was a problem hiding this comment.
💬 Approved with comments — The --preview-name change is correct and consistent across code, docs and tests. One WARNING is still open: the pinned cf and the * peer range still allow cf versions that reject the new flag.
Findings: 1 warning
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #4572.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] The pinned cf (`1.0.0-beta.1`, pnpm-workspace.yaml:117) and the open `cf: "*"` peer range still allow cf versions that reject `--preview-name`, so named Preview deploys fail with a raw yargs error.
- **File:** packages/cloudflare/src/deploy.ts:726
📋 Code Quality: ⚠️ 1 warning
The flag change is correct. One open WARNING: the cf pin and peer range still allow cf versions that reject --preview-name.
Recommendation: 📝 Minor Comments
- [WARNING] The pinned cf (
1.0.0-beta.1, pnpm-workspace.yaml:117) and the opencf: "*"peer range still allow cf versions that reject--preview-name, so named Preview deploys fail with a raw yargs error. (packages/cloudflare/src/deploy.ts:726)
View full review
The pinned cf and the peer range still allow a cf that rejects --preview-name
buildCfDeployArgs(packages/cloudflare/src/deploy.ts:726) always sends--preview-name <name>when a name is given.- The
cloudflare-vite-v2catalog still pinscf: 1.0.0-beta.1(pnpm-workspace.yaml:117). Because cf is yargs-strict, that version rejects the flag. - The
@vinext/cloudflarepeer range is still"cf": "*"(packages/cloudflare/package.json:69). Anyone on a cf release from before cloudflare/cf#519 gets a raw yargs error from a named Preview deploy.
Before merge, move the catalog pin to the first cf release that ships --preview-name, and change the peer range to >= that version.
I found nothing else in the diff. Positional arguments are rejected with a clear message, name validation still applies, and the tests cover building the arguments, the rejection and invalid names.
✅ All other checks passed
📝 Documentation: ✅ LGTM
The Worker Preview docs and the CLI help text both describe --preview-name <name>. Both also say that leaving it out makes cf use the CI or Git branch name.
Reviewed commit: 3cfc8fef
Follows cloudflare/cf#519, which replaces the positional Preview name in
cf previews deploywith--preview-name.Changes
vinext-cloudflare previews deploynow takes--preview-name <name>instead of a positional name. Leaving it out still makes cf use the CI or Git branch. A positional argument now throwsUnexpected argument: <arg>. Pass the Preview name with --preview-name <name>.buildCfDeployArgspasses the name to cf as--preview-name <name>.deploy-examplesworkflow, and the tests.This is a breaking CLI change:
vinext-cloudflare previews deploy pr-123becomesvinext-cloudflare previews deploy --preview-name pr-123.Status: draft until cf releases
cf is yargs-strict, so the cf version we pin now (
1.0.0-beta.1) rejects--preview-name, and cf versions that include #519 reject a positional name. Before this can land, thecloudflare-vite-v2catalog needs to move to the cf release that ships #519, after that release clears the 24hminimumReleaseAgegate. Until then, the static-assets-cache Worker Preview deploy in CI is expected to fail.