-
Notifications
You must be signed in to change notification settings - Fork 978
fix: prevent duplicate logging of warnings during wrangler dev server lifecycle events #10225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: prevent duplicate logging of warnings during wrangler dev server lifecycle events #10225
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 4394bbd The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
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: |
mockAccountId(); | ||
mockApiToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to mock the account id and api token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Devin, can you reply to the above question?
- Add comments explaining why controller.set() is called 3 times (to test duplicate prevention) - Add comment explaining why Analytics Engine test uses service worker format (to trigger specific warning) - Keep authentication mocks as they are required for remote mode and container tests Addresses GitHub comments from dario-piotrowicz on PR #10225 Co-Authored-By: [email protected] <[email protected]>
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
Devin, can you do this too? #10225 (comment) |
- Add comments explaining why controller.set() is called 3 times (to test duplicate prevention) - Add comment explaining why Analytics Engine test uses service worker format (to trigger specific warning) - Keep authentication mocks as they are required for remote mode and container tests Addresses GitHub comments from dario-piotrowicz on PR #10225 Co-Authored-By: [email protected] <[email protected]>
mockAccountId(); | ||
mockApiToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Devin, can you reply to the above question?
- Add comments explaining why controller.set() is called 3 times (to test duplicate prevention) - Add comment explaining why Analytics Engine test uses service worker format (to trigger specific warning) - Keep authentication mocks as they are required for remote mode and container tests Addresses GitHub comments from dario-piotrowicz on PR #10225 Co-Authored-By: [email protected] <[email protected]>
e46c528
to
df7c54d
Compare
something went quite wrong here, there are lots of files changes and conflicts, could you undo the changes that you've made after commit df7c54d? |
… lifecycle events - Updated ConfigController.ts to use logger.once.warn() for warnings that should only appear once per dev session - Updated print-bindings.ts warnOrError function to use logger.once.warn() for binding warnings - Added comprehensive tests to verify duplicate logging prevention for all warning types - Covers queue, analytics engine, service binding, container, and upstream protocol warnings - Tests verify warnings appear exactly once during multiple config resolutions Fixes #6855 Co-Authored-By: [email protected] <[email protected]>
Address GitHub comment by replacing dynamic fs imports with a proper top-level import. This follows standard Node.js patterns and improves code readability. - Replace 'const fs = await import("node:fs");' with top-level import - Use 'writeFileSync' directly instead of 'fs.writeFileSync' - Maintains same functionality while following better practices Co-Authored-By: [email protected] <[email protected]>
- Add comments explaining why controller.set() is called 3 times (to test duplicate prevention) - Add comment explaining why Analytics Engine test uses service worker format (to trigger specific warning) - Keep authentication mocks as they are required for remote mode and container tests Addresses GitHub comments from dario-piotrowicz on PR #10225 Co-Authored-By: [email protected] <[email protected]>
… function - Address GitHub comments by creating reusable helper function - Remove container warning test as functionality doesn't exist in v3 codebase - Fix TypeScript linting errors by replacing 'any' types with 'object' Co-Authored-By: [email protected] <[email protected]>
df7c54d
to
856d720
Compare
775b610
to
856d720
Compare
Fixes #6855
This PR addresses the duplicate logging issue in
wrangler dev
where warnings were being displayed multiple times during server state changes (start/stop/reload) or seemingly randomly. The fix uses the existinglogger.once.warn()
mechanism to ensure warnings only appear once per dev session.Changes Made
Core Fix
ConfigController.ts: Updated 6 warning calls to use
logger.once.warn()
instead oflogger.warn()
:print-bindings.ts: Updated the
warnOrError()
function to uselogger.once.warn()
for binding-related warningsTest Coverage
Technical Approach
The fix leverages the existing
logger.once
mechanism inpackages/wrangler/src/logger.ts
which maintains a history of logged messages to prevent duplicates. This approach is preferable to other solutions because:Review Focus Areas
High Priority:
logger.once.warn()
is the correct approach vs. addressing root cause of duplicate callswrangler dev
usage patternsMedium Priority:
logger.once
history trackingTesting Notes:
ConfigController
directly rather than fullwrangler dev
integrationaddEventListener
syntax instead of ES modulesLink to Devin run: https://app.devin.ai/sessions/328baf572c9d40c38d5f7f5256dacaa7
Requested by: @dario-piotrowicz