Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions packages/targets/deploy-workers/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,46 @@ describe('Cloudflare Workers deployment target', () => {
expect(execMock).not.toHaveBeenCalled();
});

it('rejects invalid Workers config before plan or CLI work', async () => {
await expect(adapter.build(fakeBuildContext() as any, {
name: ' ',
accountId: 'account-123',
})).rejects.toThrow('deploy-workers requires name');

await expect(adapter.ship(fakeShipContext({
dryRun: true,
}) as any, {
name: 'api-worker',
accountId: 'account/123',
})).rejects.toThrow('accountId must be a single URL-safe segment');

await expect(adapter.ship(fakeShipContext({
dryRun: true,
}) as any, {
name: 'api-worker',
accountId: 'account-123',
routes: [' '],
})).rejects.toThrow('deploy-workers requires routes[0]');

await expect(adapter.ship(fakeShipContext({
dryRun: true,
}) as any, {
name: 'api-worker',
accountId: 'account-123',
compatibilityDate: '20260521',
})).rejects.toThrow('compatibilityDate must use YYYY-MM-DD');

await expect(adapter.ship(fakeShipContext({
dryRun: true,
}) as any, {
name: 'api-worker',
accountId: 'account-123',
vars: { 'bad-key': 'value' },
})).rejects.toThrow('var "bad-key" must be a valid environment variable name');

expect(execMock).not.toHaveBeenCalled();
});

it('requires a Cloudflare API token for real deployments', async () => {
await expect(adapter.ship(fakeShipContext({
dryRun: false,
Expand Down
64 changes: 64 additions & 0 deletions packages/targets/deploy-workers/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,82 @@ interface Config {
vars?: Record<string, string>;
}

function requireText(value: string | undefined, field: string): string {
const text = value?.trim();
if (!text) throw new Error(`deploy-workers requires ${field}`);
return text;
}

function optionalText(value: string | undefined, field: string): string | undefined {
return value === undefined ? undefined : requireText(value, field);
}

function requireSegment(value: string | undefined, field: string): string {
const text = requireText(value, field);
if (/[\\/?#\s\x00-\x1F\x7F]/.test(text)) {
throw new Error(`deploy-workers ${field} must be a single URL-safe segment`);
}
return text;
}

function compatibilityDate(value: string | undefined): string | undefined {
const date = optionalText(value, 'compatibilityDate');
if (date && !/^\d{4}-\d{2}-\d{2}$/.test(date)) {
throw new Error('deploy-workers compatibilityDate must use YYYY-MM-DD');
}
Comment on lines +36 to +38

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 ^\d{4}-\d{2}-\d{2}$ only validates the format, not calendar validity. A value like '2026-13-45' satisfies the regex but is not a real date, and Wrangler would reject it at deploy time. Adding isNaN(Date.parse(date)) as a secondary guard would catch logically invalid dates early.

Suggested change
if (date && !/^\d{4}-\d{2}-\d{2}$/.test(date)) {
throw new Error('deploy-workers compatibilityDate must use YYYY-MM-DD');
}
if (date && (!/^\d{4}-\d{2}-\d{2}$/.test(date) || isNaN(Date.parse(date)))) {
throw new Error('deploy-workers compatibilityDate must use YYYY-MM-DD');
}

return date;
}

function routes(value: string[] | undefined): string[] | undefined {
return value?.map((route, index) => requireText(route, `routes[${index}]`));
}

function workerVars(value: Record<string, string> | undefined): Record<string, string> | undefined {
if (value === undefined) return undefined;
for (const [key, entryValue] of Object.entries(value)) {
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(key)) {
throw new Error(`deploy-workers var "${key}" must be a valid environment variable name`);
}
if (typeof entryValue !== 'string') {
throw new Error(`deploy-workers var "${key}" must be a string`);
}
}
return value;
}

function normalizedConfig(config: Config): Config {
return {
...config,
name: requireSegment(config.name, 'name'),
accountId: requireSegment(config.accountId, 'accountId'),
routes: routes(config.routes),
env: optionalText(config.env, 'env'),
compatibilityDate: compatibilityDate(config.compatibilityDate),
entrypoint: optionalText(config.entrypoint, 'entrypoint'),
configPath: optionalText(config.configPath, 'configPath'),
vars: workerVars(config.vars),
};
}

function workerEntry(ctx: { projectDir: string }, config: Config): string | undefined {
config = normalizedConfig(config);
if (!config.entrypoint) return undefined;
return isAbsolute(config.entrypoint) ? config.entrypoint : join(ctx.projectDir, config.entrypoint);
}

function wranglerConfig(ctx: { projectDir: string }, config: Config): string | undefined {
config = normalizedConfig(config);
if (!config.configPath) return undefined;
return isAbsolute(config.configPath) ? config.configPath : join(ctx.projectDir, config.configPath);
}

function deployEnv(ctx: { channel: string }, config: Config): string {
config = normalizedConfig(config);
return config.env ?? (ctx.channel === 'stable' ? 'production' : 'preview');
}

function deployArgs(ctx: { channel: string; projectDir: string }, config: Config, opts: { dryRun?: boolean } = {}): string[] {
config = normalizedConfig(config);
const args = ['--yes', 'wrangler', 'deploy'];
const entrypoint = workerEntry(ctx, config);
if (entrypoint) args.push(entrypoint);
Expand All @@ -43,6 +104,7 @@ function deployArgs(ctx: { channel: string; projectDir: string }, config: Config
}

function renderPlan(ctx: { channel: string; projectDir: string; version: string }, config: Config): string {
config = normalizedConfig(config);
Comment on lines 73 to +107

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant normalizedConfig calls throughout helpers

normalizedConfig is called in both the top-level entry points (build, ship) and again inside every helper (workerEntry, wranglerConfig, deployEnv, deployArgs, renderPlan). A single build invocation triggers roughly 9 normalizations. While idempotent today, this pattern obscures where validation actually occurs and makes the helpers harder to test in isolation. Consider removing the normalizedConfig call from the helpers and ensuring callers always pass an already-normalized config, or keeping validation only at the public build/ship entry points.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

return `${JSON.stringify({
provider: 'cloudflare-workers',
name: config.name,
Expand All @@ -67,13 +129,15 @@ export default defineTarget<Config>({
kind: 'web',
label: 'Cloudflare Workers',
async build(ctx, config) {
config = normalizedConfig(config);
const planPath = join(ctx.outDir, 'workers-deploy.json');
ctx.log(`wrangler deploy --dry-run - name=${config.name}`);
await mkdir(ctx.outDir, { recursive: true });
await writeFile(planPath, renderPlan(ctx, config), 'utf-8');
return { artifact: planPath };
},
async ship(ctx, config) {
config = normalizedConfig(config);
const env = deployEnv(ctx, config);
ctx.log(`wrangler deploy - name=${config.name} - env=${env}`);
if (ctx.dryRun) {
Expand Down
Loading