feat(wrangler): Add config validation for Pages

[CarmenPopoviciu]

What this PR solves / how to test

Wrangler proper has a mechanism in place through which it validates a wrangler.toml file for Workers projects. As part of adding wrangler.toml support for Pages, we need to put a similar mechanism in place, to validate a configuration file against Pages specific requirements.

This PR implements the Pages specific validation logic

Author has addressed the following

• Tests
  • TODO (before merge)
  • Included
  • Not necessary because:
• Changeset (Changeset guidelines)
  • TODO (before merge)
  • Included
  • Not necessary because:
• Public documentation
  • TODO (before merge)
  • Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/...
  • Not necessary because: not needed at this stage of the work

[changeset-bot]

🦋 Changeset detected

Latest commit: 6799c17

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
wrangler	Minor
@cloudflare/vitest-pool-workers	Patch

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

[CarmenPopoviciu]

Pete suggested this validation should be performed inside normalizeAndValidateConfig(), which would remove the need to pass in rawConfig here. I tend to agree, so will address that

[penalosa]

I'm not sure about that—this feels like the right place for it given it's doing Pages specific validation?

[petebacondarwin]

If it is Pages specific I agree with you @penalosa.

But doing it this way has a different downside, which is that this code needs to understand how RawConfig works and parse out the env property from there.

My suggestion was that it would be nice if these functions only cared about the processed Config. As such we could augment normalizeAndValidateConfig() with an extra parameter called allowedEnvironments (or something like that) which is not Pages specific per se but means that we don't need to pass the rawConfig into this function.

Moreover, this feature of normalizeAndValidateConfig() could be extended to handle the environment fallback logic that we need for Pages - e.g. something like:

const allowedEnvs = [
 { name: "production", fallbackToTopLevel: true },
 { name: "preview", fallbackToTopLevel: true },
]
normalizeAndValidateConfig(rawConfig, configPath, args, allowedEnvs);

That kind of thing... open to other ideas but it seems awkward for us to have to have special logic that messes around with RawConfig.env outside of that function.

[CarmenPopoviciu]

removed the dependency on RawConfig for now

[CarmenPopoviciu]

actually...I want to come back this. so as I am looking at implementing env inheritance for Pages, I better understand now where you're coming from @petebacondarwin. told you my brain works in baby avocado increments 😅 🥑

so assuming we would change the normalizeAndValidateConfig() fn signature to accept that extra allowedEnvs param, are you saying that the env name validation for Pages should take place inside that fn as well? If we don't, we basically risk creating that normalized config for an invalid Pages env name, only to then fail in validatePagesConfig(), which feels wasteful to me (tho IMHO we are already potentially doing wasteful config processing as we validate for Pages after we normalize, so maybe not a biggie). However, if we do, we leak Pages-specific validation things into normalizeAndValidateConfig, which is something I was hoping to prevent.

It seems to me, that either way we go abt it, there's no one way forward that feels 100% right and clean, at least to me not sure how y'all feel abt it, and there's some compromise that needs to happen between:

• keeping all Pages-specific validation code in one place
• avoid polluting normalizeAndValidateConfig with Pages things
• validating Pages against a normalized config

[penalosa]

I think my thinking around this was that it's unavoidable that we make some changes to normalizeAndValidateConfig(), but by splitting out the validation into Pages specific and workers specific we can minimise the changes to normalizeAndValidateConfig() to just the one line change outlined in the spec

[CarmenPopoviciu]

@penalosa are you referring to the one liner env inheritance change here?

[penalosa]

@CarmenPopoviciu yes

[CarmenPopoviciu]

will leave this conversation unresolved for posterity, but AFAICT, we seem to be OK with the current implementation, so leaving as is. Very much appreciate y'all's constructive thoughts around this tho <3

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8315841800/npm-package-wrangler-5166

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5166/npm-package-wrangler-5166

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8315841800/npm-package-wrangler-5166 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8315841800/npm-package-create-cloudflare-5166 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8315841800/npm-package-cloudflare-kv-asset-handler-5166

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8315841800/npm-package-miniflare-5166

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8315841800/npm-package-cloudflare-pages-shared-5166

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8315841800/npm-package-cloudflare-vitest-pool-workers-5166

Note that these links will no longer work once the GitHub Actions artifact expires.

---

wrangler@3.34.2 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20240304.2
workerd	1.20240304.0	1.20240304.0
workerd --version	1.20240304.0	2024-03-04

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[codecov]

Codecov Report

Attention: Patch coverage is 77.46479% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 71.55%. Comparing base (4ce7a5d) to head (6799c17).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5166      +/-   ##
==========================================
+ Coverage   71.46%   71.55%   +0.08%     
==========================================
  Files         309      311       +2     
  Lines       16098    16170      +72     
  Branches     4111     4137      +26     
==========================================
+ Hits        11505    11570      +65     
- Misses       4593     4600       +7     

Files	Coverage Δ
packages/wrangler/src/config/config.ts	100.00% <100.00%> (ø)
packages/wrangler/src/config/validation-pages.ts	97.61% <97.61%> (ø)
packages/wrangler/src/config/validation.ts	89.31% <58.33%> (-0.43%)	⬇️
packages/wrangler/src/config/index.ts	85.63% <33.33%> (-4.54%)	⬇️

... and 7 files with indirect coverage changes

[penalosa]

I think my thinking around this was that it's unavoidable that we make some changes to normalizeAndValidateConfig(), but by splitting out the validation into Pages specific and workers specific we can minimise the changes to normalizeAndValidateConfig() to just the one line change outlined in the spec

[CarmenPopoviciu]

@penalosa @petebacondarwin all your comments are ✅ ed. Pls let me know if there is anything else you'd like me address <3. Thank you so much for your thorough reviews!

[penalosa]

LGTM other than durable objects 🎉