feat(wrangler): Add a new wrangler preview command group for Worker Previews#12983
feat(wrangler): Add a new wrangler preview command group for Worker Previews#129831000hz wants to merge 43 commits intocloudflare:mainfrom
wrangler preview command group for Worker Previews#12983Conversation
🦋 Changeset detectedLatest commit: 3572a62 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
f9cf745 to
2ab5201
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
f165a2b to
14993e8
Compare
e4942f6 to
a65531f
Compare
penalosa
left a comment
There was a problem hiding this comment.
First pass review—my main concern at this stage is code duplication, and using existing utilities from across the codebase.
| @@ -8213,6 +8214,101 @@ describe("normalizeAndValidateConfig()", () => { | |||
| expect(diagnostics.hasWarnings()).toBe(false); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("[previews]", () => { | |||
There was a problem hiding this comment.
This is fairly minimal testing. I'd like to see tests of the inheritability of the previews field and how values in the previews field are parsed.
| * Get a human-readable display value for a binding. | ||
| * Used by both preview deployment and Previews settings formatting. | ||
| */ | ||
| export function getBindingValue(binding: Binding): string { |
There was a problem hiding this comment.
We have an existing utility for this that should be used instead: printBindings()
There was a problem hiding this comment.
printBindings() prints all bindings straight to the logger, which limits any control over formatting, etc. (perhaps somewhat intentionally).
I agree that we should not be duplicating this type of logic across the codebase, but I think we need a more flexible utility available for this sort of thing. This really ought to belong in @cloudflare/workers-utils and be consumed by printBindings().
There was a problem hiding this comment.
perhaps somewhat intentionally
It is intentional, yes—it means that every time Wrangler prints out bindings they're formatted the same. Are there specific aspects of the printBindings() format that you'd like to change? Or is this primarily about being able to wrap the output in a box?
| type: "string", | ||
| requiresArg: true, | ||
| }, | ||
| "ignore-defaults": { |
There was a problem hiding this comment.
We've discussed this at length, I know, but I'm just restating my opinion that users will find this confusing, and that there's a real potential for ending up with accidentally deployed previews with very stale config values from the dash. It also behaves completely differently to everything else in Wrangler.
There was a problem hiding this comment.
This seems like it has a lot of duplicated code from the deployment pipeline?
There was a problem hiding this comment.
as far as I could tell, there weren't low-level enough utilities we could reuse here, as the deployment pipeline code assumes it needs to prepare a multipart form-data request, whereas we need to hit the /workers/workers/... endpoints which only accept JSON, and have different schema requirements, etc.
Again, I'd love to not have to duplicate any logic here, and hope we can refactor things to share the core functionality.
There was a problem hiding this comment.
Sounds good—we'll go through this later and clean up
dffb6fc to
9830a63
Compare
…eploy - Add preview.yml: creates a Worker Preview on every PR using wrangler preview (prerelease from cloudflare/workers-sdk#12983), posts/updates a PR comment with the preview URL - Add deploy.yml: runs npm run deploy on pushes to main - Update wrangler to prerelease version from pkg.pr.new/wrangler@12983 - Replace deploy:preview script to call wrangler preview after build - Remove vite preview script (superseded by wrangler preview)
* Add GitHub Actions workflows for preview deployments and production deploy - Add preview.yml: creates a Worker Preview on every PR using wrangler preview (prerelease from cloudflare/workers-sdk#12983), posts/updates a PR comment with the preview URL - Add deploy.yml: runs npm run deploy on pushes to main - Update wrangler to prerelease version from pkg.pr.new/wrangler@12983 - Replace deploy:preview script to call wrangler preview after build - Remove vite preview script (superseded by wrangler preview) * Fix: install wrangler prerelease as a workflow step, not in package.json npm ci rejects non-semver versions (URL-based installs), so the prerelease wrangler must be installed after npm ci rather than declared in package.json. * Fix: restore clean package-lock.json without pkg.pr.new URLs The lockfile had URL-resolved entries for wrangler and its sub-deps from the prerelease install, which caused npm ci to fail with 'Invalid Version'. * Fix: capture wrangler output before propagating exit code - Use set +e to prevent bash -e from aborting before we can echo output - Run Post preview comment step with if: always() so it posts even on failure * Fix: don't post PR comment on failure, fix output interpolation in github-script - Remove if: always() so comment only posts on successful deploy - Remove raw wrangler output interpolation into JS template literal which caused SyntaxError when output contained JS keywords * Fix: add CLOUDFLARE_ACCOUNT_ID env var to both workflows Wrangler fails in non-interactive mode when multiple accounts are available and no account ID is specified. * Fix: pass branch name as preview --name to avoid detached HEAD slug GitHub Actions checks out PRs in detached HEAD state, causing git rev-parse --abbrev-ref HEAD to return 'HEAD' -> slug 'head'. Pass --name explicitly using github.head_ref instead.
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
Wrangler now reads GITHUB_HEAD_REF/GITHUB_REF_NAME natively (cloudflare/workers-sdk#12983), so we no longer need to manually derive and pass --name to the preview deploy command.
Wrangler now reads GITHUB_HEAD_REF/GITHUB_REF_NAME natively (cloudflare/workers-sdk#12983), so we no longer need to manually derive and pass --name to the preview deploy command.
…#5) Wrangler now reads GITHUB_HEAD_REF/GITHUB_REF_NAME natively (cloudflare/workers-sdk#12983), so we no longer need to manually derive and pass --name to the preview deploy command.
…#4) Wrangler now reads GITHUB_HEAD_REF/GITHUB_REF_NAME natively (cloudflare/workers-sdk#12983), so we no longer need to manually derive and pass --name to the preview deploy command.
| script_name?: string; | ||
| } | ||
|
|
||
| export type EnvBindings = Record<string, Binding>; |
There was a problem hiding this comment.
I don't understand what this type represents. The current correct discriminated type for metadata bindings is WorkerMetadataBinding from @cloudflare/workers-utils, and the new type for the new config/API shape is Binding (also from from @cloudflare/workers-utils). Previews should use one of those rather than creating a new type.
There was a problem hiding this comment.
this is:
const env: EnvBindings = {
FOO: {
type: "plain_text",
text: "foo"
},
BAR: {
type: "kv_namespace",
namespace_id: "1234567890abcdef1234567890abcdef"
}
}Where Binding here is the API representation of a binding (which Binding from "@cloudflare/util-workers" doesn't seem to quite match? e.g. CfKVNamespace has an id property rather than namespace_id...)
So in theory we could do away with EnvBindings and just use Record<string, Binding> where Binding is the shared type from @cloudflare/util-workers if it accurately reflects the API shape... But this type will come from the SDK soon anyways.
There was a problem hiding this comment.
Also in case it wasn't clear, the /workers/workers/... flavor of endpoints now/soon accept and return bindings as env?: Record<string, Binding> instead of bindings?: Array<NamedBinding>.
There was a problem hiding this comment.
Yeah, this is the issue, I think. The current API representation of bindings isn't perfect (e.g. namespace_id vs id as one example), and so there'll be more changes needed down the line rather than just changing the shape to a Record.
There was a problem hiding this comment.
Sounds good—we'll go through this later and clean up
…eview_defaults` schema We updated the API to not store a distinct "previews" value for these settings.
`cpu_ms` shouldn't be required, and it should also support `subrequests`.
b034c03 to
99a797f
Compare
This reverts commit 6eaa39f.
| isValid = | ||
| validateStreamingTailConsumers( | ||
| diagnostics, | ||
| `${field}.streaming_tail_consumers`, | ||
| previews.streaming_tail_consumers, | ||
| undefined | ||
| ) && isValid; |
There was a problem hiding this comment.
🟡 streaming_tail_consumers validated but silently dropped at runtime
The validatePreviewsConfig function accepts streaming_tail_consumers in the allowed properties list (line 4887) and validates it (line 5034), and the PreviewsConfig type includes it via EnvironmentNonInheritable. However, assemblePreviewScriptSettings in packages/wrangler/src/preview/shared.ts:310-315 only handles tail_consumers, not streaming_tail_consumers. The CreatePreviewRequestParams API type in packages/wrangler/src/preview/api.ts:108-113 also has no streaming_tail_consumers field. This means a user can configure streaming_tail_consumers inside their previews block, it passes validation without any warning, but the value is completely ignored—never sent to the API. This is a silent data loss: the user believes their streaming tail consumers are configured for previews, but they are not.
Prompt for agents
Either remove streaming_tail_consumers from the allowed properties list in validatePreviewsConfig (line 4887 of packages/workers-utils/src/config/validation.ts) and from the validation block (lines 5033-5039), OR add handling for streaming_tail_consumers in assemblePreviewScriptSettings in packages/wrangler/src/preview/shared.ts around line 310-315 and add the corresponding field to CreatePreviewRequestParams in packages/wrangler/src/preview/api.ts. The validation whitelist and runtime behavior should be consistent to avoid silent data loss.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ssembling deployment modules
…s aren't consistent across CI environments
| compatibilityDate, | ||
| compatibilityFlags, | ||
| alias: config.alias, | ||
| define: config.previews?.define ?? {}, |
There was a problem hiding this comment.
🔴 Preview bundling silently drops top-level define substitutions
In getDeploymentModules, the define option passed to bundleWorker is config.previews?.define ?? {} (packages/wrangler/src/preview/preview.ts:206). This means whenever config.previews.define is undefined (e.g. the user has a previews block for bindings but doesn't override define), the result is {} — all top-level config.define entries are silently dropped from the bundle. This causes compile-time constants to be missing in the preview worker, leading to runtime errors or incorrect behavior. The normal deploy flow at packages/wrangler/src/deploy/deploy.ts:741 correctly merges with { ...config.define, ...props.defines }. The fix should be { ...config.define, ...(config.previews?.define ?? {}) } so that top-level defines are preserved and preview-specific defines can override individual keys.
| define: config.previews?.define ?? {}, | |
| define: { ...config.define, ...(config.previews?.define ?? {}) }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I intentionally did not do this since define is a non-inheritable field.
|
@penalosa This PR should now be ready for a final review. I did several passes on feedback from Devin and resolved everything that seemed legit, but let me know if it flagged anything you think is still important to address. |
Refs IAC-362
This PR introduces a
wrangler previewfamily of commands for creating Preview deployments and managing Previews settings. This feature is currently limited to private beta status.New commands
wrangler previewwrangler preview deletewrangler preview settingswrangler preview settings updatewrangler preview secret putwrangler preview secret deletewrangler preview secret listwrangler preview secret bulkIt also introduces a
previewsproperty to the Wrangler config schema which includes all binding-related fields as well aslimits,observability, andlogpush. These fields all adhere to their existing heritability traits, withpreviewsitself being inheritable. This property allows users to specify an alternate configuration for their Worker's Preview deployments.Also included are some new box drawing utilities, though we may want to swap these out for a library like https://github.com/sindresorhus/boxen in the future.