feat(wrangler): Implement environment inheritance for Pages configuration #5234
Conversation
🦋 Changeset detectedLatest commit: 338c044 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
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/8356206987/npm-package-wrangler-5234You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5234/npm-package-wrangler-5234Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8356206987/npm-package-wrangler-5234 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8356206987/npm-package-create-cloudflare-5234 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8356206987/npm-package-cloudflare-kv-asset-handler-5234npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8356206987/npm-package-miniflare-5234npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8356206987/npm-package-cloudflare-pages-shared-5234npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8356206987/npm-package-cloudflare-vitest-pool-workers-5234Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
CarmenPopoviciu
force-pushed
the
carmen/env-inheritance
branch
from
2501658
to
504217d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5234 +/- ##
==========================================
+ Coverage 71.64% 71.68% +0.03%
==========================================
Files 312 313 +1
Lines 16217 16230 +13
Branches 4149 4151 +2
==========================================
+ Hits 11619 11634 +15
+ Misses 4598 4596 -2
|
CarmenPopoviciu
force-pushed
the
carmen/env-inheritance
branch
from
504217d
to
23a3645
Compare
CarmenPopoviciu
changed the title
CarmenPopoviciu
changed the title
CarmenPopoviciu
force-pushed
the
carmen/env-inheritance
branch
from
23a3645
to
45b648c
Compare
| }) | ||
| ); | ||
|
|
||
| /*======================================*/ |
There was a problem hiding this comment.
just calling this test out to highlight that this happens because we do the Pages-specific validation after normalizeAndValidateConfig(). But in an e2e flow, this unsupported named env will fail Pages validation and wrangler will err out
There was a problem hiding this comment.
// TODO @CarmenPopoviciu add a comment to explain the above, if we decide to keep this test as part of the suite
There was a problem hiding this comment.
comments added ✅
packages/wrangler/src/__tests__/helpers/generate-wrangler-config.ts
Outdated
Show resolved
Hide resolved
| } from "./helpers/generate-wrangler-config"; | ||
| import type { RawConfig, RawEnvironment } from "../config"; | ||
|
|
||
| describe("normalizeAndValidateConfig()", () => { |
There was a problem hiding this comment.
are we OK with these tests living in a separate file? it's rather unconventional to test the same fn over 2 separate test suites, but:
configuration.test.tsis already 5k lines long so had mixed feelings abt adding more to it.- this made it easy to write the tests
- this way we keep Pages specific tests in one place. Granted we could achieve that via a dedicated
describeblock insideconfiguration.test.ts
I have no strong opinions, but if anyone else does, happy to readjust.
CarmenPopoviciu
force-pushed
the
carmen/env-inheritance
branch
from
45b648c
to
f2c9606
Compare
| import type { RawConfig, RawEnvironment } from "../config"; | ||
|
|
||
| describe("normalizeAndValidateConfig()", () => { | ||
| describe("Pages configuration", () => { |
There was a problem hiding this comment.
Is this describe() necessary? It seems like describe("normalizeAndValidateConfig()") has no other children
There was a problem hiding this comment.
it's not necessary per se, it's merely to have some logical grouping in place, and separate the Pages test suite from the suite in configuration.test.ts. We already have a describe("normalizeAndValidateConfig()",...) block in place in configuration.test.ts, and since we're duplicating that suite name here, having an extra grouping layer felt like a clean approach. If this feels like overkill, we could rename to top level grouping to smth like normalizeAndValidateConfig() for Pages and get rid of the secondary grouping layer
CarmenPopoviciu
force-pushed
the
carmen/env-inheritance
branch
from
f2c9606
to
148e627
Compare
To make it easier for users to write config files for Pages, Wrangler will not require both of those environments to be explicitly defined in the `wrangler.toml` file. If either `[env.production]` or `[env.preview]` is left unspecified, Wrangler will use the top-level Wrangler environment when targeting that named Pages environment. This commit implements the aforementioned environment inheritance logic for Pages projects.
CarmenPopoviciu
force-pushed
the
carmen/env-inheritance
branch
from
148e627
to
338c044
Compare
What this PR solves / how to test
For Pages, Wrangler will not require both of supported named environments ("preview" | "production") to be explicitly defined in the config file. If either
[env.production]or[env.preview]is left unspecified, Wrangler will use the top-level environment when targeting that named Pages environment.Author has addressed the following