[wrangler] Surface _redirects validation warnings at deploy time (#14694)#14709
[wrangler] Surface _redirects validation warnings at deploy time (#14694)#14709allocsys wants to merge 12 commits into
Conversation
🦋 Changeset detectedLatest commit: 175cf65 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
allocsys
changed the title
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…udflare#14694 review comment) config.assets?.html_handling was already available in this function but wasn't threaded through to the new deploy-time parseRedirects() call, so htmlHandling defaulted to undefined -- which parseRedirects treats as "enabled" -- causing false "infinite loop detected" warnings for users who set html_handling: "none" on rules like `/ /index.html`. Now passes the same option the Vite plugin already passes correctly.
…infinite-loop warning at deploy time Covers the review finding on cloudflare#14709 -- parseRedirects() previously ran with htmlHandling always undefined at deploy time (treated as enabled), so a rule like `/ /index.html` was flagged as an infinite loop even for projects that explicitly disabled HTML handling. Adds a test confirming no warning is produced now that config.assets?.html_handling is threaded through, plus a sanity check that the real infinite-loop case is still caught when HTML handling is left at its default.
|
Fixed in 018173c — Added two regression tests in 627a9d2:
Both commits are pushed to this PR's branch. Thanks for catching this! |
| `wrangler deploy` now parses `_redirects` for validation purposes and warns about any invalid rules, using the same messages already shown by `wrangler dev`. This does not change what gets uploaded — the raw file is still uploaded as-is, and the asset worker remains the authoritative parser at runtime. | ||
|
|
||
| <!-- trigger fork CI --> |
There was a problem hiding this comment.
This comment can be removed
| `wrangler deploy` now parses `_redirects` for validation purposes and warns about any invalid rules, using the same messages already shown by `wrangler dev`. This does not change what gets uploaded — the raw file is still uploaded as-is, and the asset worker remains the authoritative parser at runtime. | |
| <!-- trigger fork CI --> | |
| `wrangler deploy` now parses `_redirects` for validation purposes and warns about any invalid rules, using the same messages already shown by `wrangler dev`. This does not change what gets uploaded — the raw file is still uploaded as-is, and the asset worker remains the authoritative parser at runtime. |
| expect(std.warn).toMatchInlineSnapshot(`""`); | ||
| }); | ||
|
|
||
| it("should warn about invalid _redirects rules at deploy time without altering what is uploaded (#14694)", async ({ |
There was a problem hiding this comment.
We probably don't need the issue reference in the title of this test name.
| it("should warn about invalid _redirects rules at deploy time without altering what is uploaded (#14694)", async ({ | |
| it("should warn about invalid _redirects rules at deploy time without altering what is uploaded", async ({ |
| expect(std.warn).toContain("Ignoring duplicate rule for path /foo."); | ||
| }); | ||
|
|
||
| it("should not warn about a false infinite-loop when html_handling is 'none' (#14709 review)", async ({ |
There was a problem hiding this comment.
| it("should not warn about a false infinite-loop when html_handling is 'none' (#14709 review)", async ({ | |
| it("should not warn about a false infinite-loop when html_handling is 'none'", async ({ |
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
NuroDev
left a comment
NuroDev
left a comment
There was a problem hiding this comment.
Can you also update the PR description to reflect the repository template so we have details on tests, docs, etc.
| if (_redirects) { | ||
| const parsedRedirects = parseRedirects(_redirects, { | ||
| htmlHandling: config.assets?.html_handling, | ||
| }); | ||
| if (parsedRedirects.invalid.length > 0) { | ||
| logger.warn( | ||
| formatInvalidRedirectsWarning( | ||
| parsedRedirects.invalid, | ||
| REDIRECTS_FILENAME | ||
| ) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Invalid redirect-file warnings shown twice during local development
Redirect-file validation warnings are now emitted inside a shared resolution step (parseRedirects/logger.warn at packages/deploy-helpers/src/deploy/helpers/assets.ts:567-579) that also runs during local development, where the same warnings are already produced separately, so a developer sees each invalid-redirect warning twice.
Impact: During wrangler dev (and in preview / platform-proxy tooling), users get duplicate and unexpected warnings about invalid _redirects rules that previously appeared only once.
Why the shared code path double-fires the warning
The new block was added to resolveAssetOptions() in packages/deploy-helpers/src/deploy/helpers/assets.ts:567-579. That function is not deploy-only: packages/wrangler/src/assets.ts:132 (getAssetsOptions) calls it, and getAssetsOptions is invoked from the dev config resolver packages/wrangler/src/api/startDevWorker/ConfigController.ts:375, from packages/wrangler/src/preview/commands.ts:51, and from packages/wrangler/src/api/integrations/platform/index.ts (getPlatformProxy).
In wrangler dev, DevEnv.startWorker first calls initDeployHelpersContext (packages/wrangler/src/api/startDevWorker/DevEnv.ts:41) so the deploy-helpers logger routes to wrangler's logger, then this.config.set(...) runs ConfigController, which calls getAssetsOptions → resolveAssetOptions, emitting the invalid-redirect warning. Separately, Miniflare's asset plugin parses the same _redirects file and emits the identical warning via constructRedirects (packages/miniflare/src/plugins/assets/index.ts:150-159). The result is two copies of the warning for every invalid rule. Before this PR resolveAssetOptions did not parse _redirects, so dev showed the warning only once.
Prompt for agents
The new _redirects validation warning was added to resolveAssetOptions() in packages/deploy-helpers/src/deploy/helpers/assets.ts (lines 567-579). resolveAssetOptions is a shared function: it is called via packages/wrangler/src/assets.ts getAssetsOptions(), which runs not only in the deploy/versions-upload paths but also during wrangler dev (packages/wrangler/src/api/startDevWorker/ConfigController.ts around line 375), preview (packages/wrangler/src/preview/commands.ts), and getPlatformProxy (packages/wrangler/src/api/integrations/platform/index.ts). During wrangler dev, Miniflare's assets plugin already parses _redirects and emits the same invalid-rule warnings via constructRedirects (packages/miniflare/src/plugins/assets/index.ts around line 150). As a result, the new code causes the same warnings to be printed twice in local dev (and also surfaces them unexpectedly in preview/platform-proxy contexts). The intent of the PR is to surface these warnings only at deploy time. Consider gating the validation so it only runs on the deploy/versions-upload path — e.g. move the parse-and-warn logic out of the shared resolveAssetOptions into the deploy-specific callers (deploy.ts / versions-upload.ts), or add an explicit opt-in parameter to resolveAssetOptions that only the deploy paths pass. Ensure the existing deploy tests still pass and that dev no longer double-warns.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… appears in the wrangler changelog, per Devin review feedback on PR cloudflare#14709
|
Good catch — fixed in 175cf65. |
|
@NuroDev The PR description now includes the Tests and Documentation sections from the template — could you take another look when you get a chance? |
3 participants
Fixes #14694
What this PR solves
Wrangler's deploy-time
_redirectsvalidation didn't respectassets.html_handlingwhen checking for infinite-loop redirect rules. This caused a false "infinite loop" warning for setups likehtml_handling: "none"with a rule such as/ /index.html, since the validator didn't account for HTML handling being disabled.Fix
parseRedirects()inresolveAssetOptions()now passeshtmlHandling: config.assets?.html_handling, matching the behavior already used by the Vite plugin (packages/vite-plugin-cloudflare/src/asset-config.ts).Tests
html_handling: "none"+ a rule like/ /index.htmlno longer produces a false "infinite loop" warning at deploy timeDocumentation