[wrangler] Add upgrade hint to unexpected configuration field warnings#11800
[wrangler] Add upgrade hint to unexpected configuration field warnings#11800petebacondarwin merged 10 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 6e3f783 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 |
|
Claude finished @southpolesteve's task —— View job Changeset Review✅ All changesets look good The changeset
The changeset follows all guidelines from |
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: |
vicb
left a comment
There was a problem hiding this comment.
I don't find "If this is a new configuration option, please update your Wrangler version." helpful.
i.e. what if not?
It could be a typo, you could already be on the latest wrangler, ...
Maybe one thing we could do instead is to validate the config against the schema and error early?
Alternative suggestions? Something needs to change about the current error. Its a dead end. Not clear what the fix is and we should at least guide the user in some direction. It could be other things but suggesting upgrading wrangler is not an unhelpful suggestion. |
904e5cf to
92cc8f4
Compare
My alternative suggestion would be to validate the config against the schema and error if there is a mismatch. IMO we should not "please update your Wrangler version" because this might not solve the issue and adding more code (that has to be maintained) for something that is not very helpful is a burden. |
|
Looking at the diffs: This PR is only adding noise here |
petebacondarwin
left a comment
There was a problem hiding this comment.
I think the idea of putting up a message (especially as it only happens once and only if there is a newer version of Wrangler available) is a reasonable UX improvement.
I think the message can be better - I made a suggestion.
But also I think we could/should look at doing some kind of fuzzy matching on the unexpected option like:
"Unexpected option trcing, did you mean tracing?"
Triage notes (stale PR review)The core idea (showing an upgrade hint when unexpected config fields are found) is sound, but the implementation needs rework per review feedback from @petebacondarwin and @vicb. Required changes:
Estimated effort: ~2-3 hours |
|
/bonk can you apply the fixes mentioned in @petebacondarwin's last comment? |
There was a problem hiding this comment.
Review Summary
The core idea — hinting users to upgrade Wrangler when unexpected config fields are found — is a reasonable UX improvement. However, the implementation needs rework to address the feedback from @petebacondarwin and @vicb. The PR also has merge conflicts that need resolving.
Issues to address
1. Replace magic string matching with a structured flag
The current check at packages/wrangler/src/config/index.ts:93:
if (diagnostics.renderWarnings().includes("Unexpected fields found")) {is fragile — it breaks if the warning message text is ever reworded. Both @vicb and @petebacondarwin flagged this.
Suggested fix: Add a hasUnexpectedFields boolean property to the Diagnostics class in packages/workers-utils/src/config/diagnostics.ts. Set it to true inside validateAdditionalProperties() in packages/workers-utils/src/config/validation-helpers.ts:632 when unexpected fields are detected. Then propagate it recursively (like hasErrors()/hasWarnings()). In config/index.ts, check diagnostics.hasUnexpectedFields (or a recursive getter) instead of string-matching.
// In diagnostics.ts
export class Diagnostics {
errors: string[] = [];
warnings: string[] = [];
children: Diagnostics[] = [];
hasUnexpectedFields: boolean = false;
// ... add a recursive check method similar to hasErrors()
}
// In validation-helpers.ts, inside validateAdditionalProperties:
if (restPropSet.size > 0) {
diagnostics.hasUnexpectedFields = true;
// ...existing warning push...
}
// In config/index.ts:
if (diagnostics.hasUnexpectedFields) {
void logUpdateHintIfAvailable();
}Since Diagnostics is not currently exported from @cloudflare/workers-utils, you'd either need to export it or just rely on the structural type (the property will be accessible either way since normalizeAndValidateConfig returns it).
2. Reword the message
Per @petebacondarwin's suggestion, the message should avoid the word "new" (which is misleading if the user is on a very old version). Something like:
"There is a newer version of Wrangler available (current: X.Y.Z, latest: A.B.C). Try upgrading, as it might support this configuration option."
This is more precise and actionable.
3. De-duplicate the check between readConfig and readPagesConfig
The same check-and-hint block is copy-pasted in both functions (lines 92-95 and 164-167). Extract the warning-logging logic into a shared helper, e.g.:
function logWarningsWithUpgradeHint(diagnostics: Diagnostics) {
logger.warn(diagnostics.renderWarnings());
if (diagnostics.hasUnexpectedFields) {
void logUpdateHintIfAvailable();
}
}Then call logWarningsWithUpgradeHint(diagnostics) in both readConfig and readPagesConfig.
4. Resolve merge conflicts
The PR is in CONFLICTING state. ConfigBindingOptions was moved to @cloudflare/workers-utils on main, so the local type definition added in this PR (lines 36-51) needs to be removed. Rebase onto current main to resolve.
5. Consider interaction with the banner
printWranglerBanner() already shows "(update available X.Y.Z)" when a newer version exists. The logUpdateHintIfAvailable() function uses the same updateCheck() call (which is memoized, so no extra network request). However, the hint message and the banner serve different purposes — the banner is a general nudge, while this hint is contextual advice tied to the specific unexpected-field warning. This is fine, but worth noting in a code comment so future maintainers understand why both exist.
|
I've posted a review on PR #11800 requesting changes. Here's a summary of the feedback:
|
When Wrangler encounters unexpected fields in the configuration file, it now suggests updating Wrangler as a potential solution. This helps users who may be using configuration options that were added in a newer version of Wrangler.
When there are unexpected field warnings, also log the current wrangler version and the latest available version (if an update is available). This helps users know if they're behind and should upgrade.
Instead of using void with an async function (which could print after other output or not at all), use a synchronous getter that returns the cached latest version if the update check has already completed. This is reliable because the banner kicks off the update check early.
The wrangler banner already shows '(update available X.X.X)' when a newer version exists. Adding version info after the warning was redundant. Keep only the simple hint message in the warning itself.
The indentText function in diagnostics.ts already adds 2 spaces to continuation lines, so the source message should not have leading spaces. This fixes the CI test failure.
Instead of always showing the upgrade hint for unexpected config fields, only display it when we detect that a newer version of Wrangler exists. This avoids confusing users who are already on the latest version.
- Add `hasUnexpectedFields` boolean property and `hasUnexpectedFieldsInTree()` recursive method to the `Diagnostics` class, mirroring `hasErrors()` - Set `hasUnexpectedFields = true` in `validateAdditionalProperties()` instead of relying on magic string matching against rendered warning output - Export `Diagnostics` class from `@cloudflare/workers-utils` - Extract `logWarningsWithUpgradeHint()` shared helper in wrangler config, eliminating the copy-paste between `readConfig` and `readPagesConfig` - Reword the hint message per Pete's suggestion: 'There is a newer version of Wrangler available (current: X.Y.Z, latest: A.B.C). Try upgrading, as it might support this configuration option.' - Remove the locally-defined `ConfigBindingOptions` type from wrangler (it already exists in and is exported from `@cloudflare/workers-utils`) - Add tests covering: hint shown when update available, hint suppressed when no update available, hint suppressed for non-unexpected-field warnings
a8ceb52 to
31124e1
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
✅ All changesets look good |
107c5f8 to
2a2ee1d
Compare
Summary
When Wrangler encounters unexpected fields in the configuration file, users often don't know why. This PR improves the warning message to suggest updating Wrangler as a potential solution, since the field might be valid in a newer version — but only when a newer version is actually available, and only via a structured flag rather than fragile string matching.
Before:
After (when a newer Wrangler version is available):
No hint is shown when the user is already on the latest version.
Implementation notes
hasUnexpectedFields: booleanproperty andhasUnexpectedFieldsInTree()recursive method to theDiagnosticsclass in@cloudflare/workers-utils, mirroring the existinghasErrors()/hasWarnings()pattern. This replaces the previous fragilerenderWarnings().includes("Unexpected fields found")string check.validateAdditionalProperties()now setsdiagnostics.hasUnexpectedFields = truewhen unknown fields are found.logWarningsWithUpgradeHint()helper inwrangler/src/config/index.ts, eliminating duplication betweenreadConfigandreadPagesConfig.ConfigBindingOptionstype definition fromwrangler/src/config/index.ts— it already exists in and is exported from@cloudflare/workers-utils.