Refactor BaseController-derived classes to use DevEnv as message bus #11244
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: ff0b639 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 |
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
Note: This PR is an internal refactoring (not a bug fix), so it doesn't need to be backported to v3-maintenance. The "Open backport PR for patches" CI check is failing, but this can be resolved by adding the @petebacondarwin - Could you please add the |
petebacondarwin
force-pushed
the
devin/1762933518-refactor-controller-message-bus
branch
2 times, most recently
from
1a2e6fc to
23a0503
Compare
|
I ran the unit tests that hit these controllers 100 times in CI. For example https://github.com/cloudflare/workers-sdk/actions/runs/19330332472/job/55291526600?pr=11244. The ConfigController.test.ts never flaked (even on multiple runs of 100). What did flake (about 1 in 30 times) was the BundleController.test.ts but this is a known flake that is not related to event handling. (The Windows tests always failed in these runs because I was using bash for-loops) |
|
CI Update: macOS test failure appears to be an unrelated flake. The failure is in Evidence it's a flake:
Waiting for Tests (Windows) to complete, then will rerun the macOS tests per repo guidelines for single-OS failures. |
packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts
Show resolved
Hide resolved
packages/wrangler/src/__tests__/api/startDevWorker/BundleController.test.ts
Show resolved
Hide resolved
packages/wrangler/src/__tests__/api/startDevWorker/LocalRuntimeController.test.ts
Show resolved
Hide resolved
CI Failure AnalysisTwo CI checks failed on the previous commit, but both appear to be unrelated flakes: 1. C3 (yarn, Linux) - Framework test failure
2. Wrangler (v20, Windows) - e2e/dev.test.ts failure
Evidence these are flakes:
Latest commit improvements:I've pushed an improved version that preserves the original logger ordering using Would appreciate if maintainers could rerun the two failing checks to confirm they're flakes. Thanks! |
| afterEach(() => { | ||
| logger.debug("tearing down"); | ||
| await controller.teardown(); | ||
| logger.debug("teardown complete"); | ||
| logger.resetLoggerLevel(); | ||
| return controller.teardown().finally(() => { | ||
| logger.resetLoggerLevel(); | ||
| }); |
There was a problem hiding this comment.
Actually this is not an improvement (to return the promise).
When there are afterEach() blocks that have multiple statements then just use async/await syntax.
Only use the "return the promise" approach when there is a simple one statement block such as:
afterEach(() => teardown(xxx));
There was a problem hiding this comment.
Got it! I've reverted to using async/await since this block has multiple statements. I've also added try/finally to ensure the logger reset runs even if teardown throws:
afterEach(async () => {
logger.debug("tearing down");
try {
await controller.teardown();
} finally {
logger.resetLoggerLevel();
}
});This follows your guidance: use "return promise" only for simple one-statement blocks (like BundleController.test.ts), and use async/await for multi-statement blocks.
Pushing the change now.
Replace Node.js EventEmitter pattern with explicit message bus dispatch pattern where DevEnv acts as central coordinator. This makes event routing traceable through a single dispatch() method with explicit switch statement routing. Key changes: - Add dispatch() method to DevEnv with explicit event routing - Remove EventEmitter from BaseController and derived classes - Update all controllers to call devEnv.dispatch() instead of emit() - Handle circular dependency by making devEnv parameter optional in constructors - Add type-only import of DevEnv in BaseController to avoid runtime circular deps This improves traceability from external triggers to error handlers by centralizing all event routing in one location. Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
The devEnv property needs to be public so DevEnv constructor can set it when controllers are passed in to handle circular dependency. Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
Tests now listen to events on DevEnv instead of directly on ConfigController since controllers no longer extend EventEmitter. Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
…ptional Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
- Remove constructors that only call super() from BundlerController, ConfigController, LocalRuntimeController, ProxyController, and RemoteRuntimeController - Update BundlerController.test.ts to use DevEnv message bus pattern - Update LocalRuntimeController.test.ts to use DevEnv message bus pattern - Remove unused DevEnv import from BundlerController These constructors were redundant because they only called super() with no additional logic. TypeScript automatically inherits the parent constructor when no constructor is defined. Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
In multiworker scenarios, the same MultiworkerRuntimeController instance is passed to multiple DevEnv constructors (primary + auxiliary). Each DevEnv constructor calls setDevEnv() on the shared runtime, which was causing the last auxiliary DevEnv to overwrite the primary DevEnv binding. This caused runtime events (reloadComplete, etc.) to be dispatched to the wrong DevEnv (one with a NoOpProxyController), leading to test timeouts. The fix makes setDevEnv() idempotent by adding a #devEnvSet flag that ensures devEnv is only set the first time, so the runtime always dispatches events to the primary DevEnv. Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
- Create ControllerBus interface with dispatch() method - Move ControllerEvent type to BaseController.ts - Update BaseController to require bus parameter in constructor - Remove setDevEnv() method and #devEnvSet flag - Update DevEnv to implement ControllerBus interface - Remove all setDevEnv() calls from DevEnv constructor - Update all controller event dispatchers to use this.bus - Fix multiworker wiring in start-dev.ts: - Create primary DevEnv first with empty runtimes array - Pass primary DevEnv as bus to MultiworkerRuntimeController - Assign runtime to primary DevEnv after creation - Create auxiliary DevEnvs with runtime, then replace proxy This eliminates the optional devEnv parameter and setDevEnv() method, making the architecture cleaner and fixing multiworker test failures. Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
- Create FakeBus test helper that implements ControllerBus interface - FakeBus provides typed waitFor() helper to wait for specific events - Refactor ConfigController.test.ts to instantiate controller directly with FakeBus - Refactor BundleController.test.ts to instantiate controller directly with FakeBus - Refactor LocalRuntimeController.test.ts to instantiate controller directly with FakeBus - Remove DevEnv instantiation from unit tests for better test isolation - All tests pass locally (26 tests total across 3 test files) Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
Replace 'w.resolve(event); break;' with 'return w.resolve(event);' for cleaner code as suggested by @petebacondarwin. This combines the resolve and exit into a single statement, making the intent clearer and eliminating the unnecessary break. Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
petebacondarwin
force-pushed
the
devin/1762933518-refactor-controller-message-bus
branch
from
e9b1d4e to
ff0b639
Compare
petebacondarwin
deleted the
devin/1762933518-refactor-controller-message-bus
branch
3 participants
Devin PR requested by pbacondarwin@cloudflare.com
Summary
Refactors BaseController-derived classes to use DevEnv as a message bus instead of the node.js EventEmitter pattern. This makes it easier to trace the path from external triggers to error handlers by centralizing all event routing in a single
dispatch()method.Changes
Core Refactoring
ControllerBusinterface (withdispatch()method) in constructor instead of optional parameterControllerBuswith centraldispatch()method that explicitly routes all eventsthis.emit(event)tothis.bus.dispatch(event)for event dispatchingDevEnv.dispatch()switch statement, making the flow from trigger to handler traceableTest Improvements
fake-bus.ts) that implementsControllerBusinterfaceFakeBusinstead of creating fullDevEnvinstanceswaitFor<T>(type, predicate?, timeout?)helper for waiting on specific events in testsMultiworker Fix
Review Focus Areas
Event Routing Completeness: Verify all event types in
DevEnv.dispatch()switch statement match the original EventEmitter wiring (configUpdate, bundleStart, bundleComplete, reloadStart, reloadComplete, devRegistryUpdate, previewTokenExpired, error)Multiworker Wiring: The multiworker scenario has unusual wiring due to circular dependency fix - verify this works correctly in
start-dev.tsError Handling: Error handling logic moved from
emitErrorEvent()tohandleErrorEvent()- verify all error cases still work (MiniflareCoreError, ProxyController errors, ConfigController ParseError)Test Isolation: FakeBus changes how tests work - verify the fake implementation matches expected behavior and all 26 tests pass
Link to Devin run: https://app.devin.ai/sessions/d0eb1a98ca824fcf85d65e8dade2a1f3