Continue event bus support for devtools#1028
Open
SeanCassiere wants to merge 26 commits into
Open
Conversation
Replace the demo-local event bus observers with the shared observeLogger helper from mosaic-core. This keeps the dev pages aligned with the public event-bus logging path instead of duplicating logging logic in each demo. The gallery query log checkbox now installs and removes the observer cleanly, so repeated toggles do not stack listeners. The dev pages also use vg.coordinator() consistently when updating coordinator settings.
SeanCassiere
force-pushed
the
devtools-event-bus-takeover
branch
from
83391d6 to
6753cda
Compare
Drop the unused client-connect event type, event class, init interface, and event map entries from the core event definitions. The event is not currently emitted or observed, and keeping it in the public-looking event model implies a client lifecycle API that this PR does not define yet.
Rename concrete event classes with a Mosaic prefix so the public API is consistent and avoids collisions with DOM event names such as ErrorEvent. Keep the shared event base internal, expose a singular MosaicEvent union, and export the event classes and related event types from the core package entrypoint for event bus consumers.
Remove the observe/unobserve aliases from ObserveDispatch and type the standard addEventListener/removeEventListener methods instead. This keeps the event bus aligned with the existing Dispatch API while preserving event-type-specific callback inference for Mosaic events. Update the logger helper and query manager tests to use the standard listener methods.
SeanCassiere
commented
May 14, 2026
SeanCassiere
commented
May 14, 2026
SeanCassiere
force-pushed
the
devtools-event-bus-takeover
branch
from
9f57e20 to
48848b9
Compare
There was a problem hiding this comment.
Pull request overview
Continues PR #950's work to replace the coordinator's logger with a lightweight, typed event bus that can power devtools. The previous Logger plumbing on Coordinator and QueryManager is removed in favor of a synchronous ObserveDispatch carrying MosaicQueryStart/QueryEnd/Warning/Error events, with a new observeLogger() helper that reproduces the old console-style logging on top of the bus.
Changes:
- Factor
AsyncDispatchinto an abstractDispatchbase and addObserveDispatch(synchronous, typed) plus aMosaicEventMapand event classes inEvents.ts. - Remove
logger/logQueriesfromCoordinatorandQueryManager; emit typed events instead, and addobserveLoggerto bridge the bus to a console-likeLogger. - Update widget, dev harness, and tests to consume
observeLogger; add tests for QueryStart/End emission (including cached requests) and observeLogger subscribe/unsubscribe.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mosaic/core/src/Events.ts | New typed event classes and MosaicEventMap. |
| packages/mosaic/core/src/util/AsyncDispatch.ts | Extracts abstract Dispatch base; keeps async behavior in AsyncDispatch via parallel _entries map. |
| packages/mosaic/core/src/util/ObserveDispatch.ts | New synchronous, typed dispatcher used as the coordinator's event bus. |
| packages/mosaic/core/src/Coordinator.ts | Removes logger API, owns eventBus, emits errors from updateClient. |
| packages/mosaic/core/src/QueryManager.ts | Removes logger fields; emits QueryStart/QueryEnd/Warning/Error via optional eventBus. |
| packages/mosaic/core/src/preagg/PreAggregator.ts | Replaces mc.logger().error with emitting MosaicErrorEvent; formatting-only changes elsewhere. |
| packages/mosaic/core/src/logger.ts | New observeLogger() that subscribes to bus and logs in old format. |
| packages/mosaic/core/src/index.ts | Re-exports event types and observeLogger. |
| packages/mosaic/core/src/util/void-logger.ts | Deleted; no longer needed. |
| packages/mosaic/core/test/coordinator.test.ts | Drops logger: null from constructor options; adds observeLogger test. |
| packages/mosaic/core/test/query-manager.test.ts | Adds QueryStart/QueryEnd emission tests including cached path. |
| packages/vgplot/widget/src/index.js | Switches to observeLogger(mc, console) with cleanup. |
| dev/setup.ts, dev/index.html, dev/query/index.html | Wires observeLogger into the dev harness to replace logQueries. |
| docs/api/core/coordinator.md | Minor list-style change and a TODO marker; doc still references removed logger API. |
Comments suppressed due to low confidence (1)
docs/api/core/coordinator.md:54
- The Coordinator documentation is now out of date. The
loggerconstructor option (line 20) and the entirecoordinator.logger()section (lines 45–54) describe APIs that this PR removed fromCoordinator. They should be removed and replaced with documentation for the newobserveLoggerexport andcoordinator.eventBus. The inline<!-- TODO: Discuss what happens here -->comment also indicates this section is intentionally left unresolved and should be addressed before merging.
- _logger_: The logger to use, defaults to `console`.
- _cache_: Boolean flag to enable/disable query caching (default `true`).
- _consolidate_ Boolean flag to enable/disable query consolidation (default `true`).
- _preagg_: Pre-aggregation options object. The _enabled_ flag (default `true`) determines if pre-aggregation optimizations should be used when possible. The _schema_ option (default `'mosaic'`) indicates the database schema in which materialized view tables should be created for pre-aggregated data.
## databaseConnector
`coordinator.databaseConnector(connector)`
Get or set the [_connector_](./connectors) used by the coordinator to issue queries to a backing data source.
## connect
`coordinator.connect(client)`
Connect a [_client_](./client) to this coordinator.
Upon connection, the [client lifecycle](/core/) will initiate.
If the client exposes a `filterBy` selection, the coordinator will handle updates to the client when the selection updates.
## disconnect
`coordinator.disconnect(client)`
Disconnect the [_client_](./client) from the coordinator and remove all update handling.
## logger
`coordinator.logger(logger)`
<!-- TODO: Discuss what happens here -->
Get or set the coordinator's logger.
The logger defaults to the standard JavaScript `console`.
A logger instance must support `log`, `info`, `warn`, and `error` methods.
If set to `null`, logging will be suppressed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reintroduce the Coordinator logger constructor option and logger() getter/setter as a compatibility layer over the new event bus logging path. The restored API keeps existing logger, custom logger, and null suppression behavior working while still routing query lifecycle logs through observeLogger. Add coordinator tests covering constructor logger configuration, logger replacement without duplicate subscriptions, and disabling logging with logger(null).
SeanCassiere
force-pushed
the
devtools-event-bus-takeover
branch
from
48848b9 to
e72d949
Compare
Make the coordinator event bus the authoritative observation surface by marking it readonly and wiring custom query managers through an internal QueryManager event-bus hook. This removes the public mutable QueryManager eventBus field so query managers act only as event producers, while observers continue to attach to Coordinator.eventBus. Add a regression test covering custom manager construction to ensure query lifecycle events still flow through the coordinator-owned bus.
Keep callback registration in the shared Dispatch callback map and store only async queue state in AsyncDispatch entries. This avoids stale per-entry callback sets when an event type is emitted before listeners are added. Add focused AsyncDispatch coverage for listener registration, removal, queue replacement, queue filtering, and cancellation behavior.
Drop the ts-expect-error on Selection.emitQueueFilter now that the dispatch queue filter hook is typed against the dispatched value type. This keeps Selection aligned with the generic AsyncDispatch contract without carrying an unnecessary TypeScript suppression.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/mosaic/core/src/Coordinator.ts:92
- The default constructor argument
logger = consolecombined with the unconditionalthis.logger(logger)call below means every Coordinator now installs a console observer by default that emits aconsole.groupCollapsed/log/groupEndfor every query. Previously, the console logger was installed by default but query SQL was only printed whenlogQueries(true)was set explicitly, so default verbosity was much lower. This is a noticeable behavior change for existing consumers; please confirm this is intended and consider documenting it in the release notes, or default tonullhere and havedev/index.htmlopt in explicitly (which it already does viasetQueryLog).
const {
logger = console,
manager = new QueryManager(),
cache = true,
consolidate = true,
preagg = {},
} = options;
this.eventBus = new ObserveDispatch<MosaicEventMap>();
this.manager = manager;
this.manager.setEventBus(this.eventBus);
this.manager.cache(cache);
this.manager.consolidate(consolidate);
this.databaseConnector(db);
this.logger(logger);
Add query ids to query lifecycle events and make QueryEnd the terminal event for both successful and failed query attempts. QueryEnd now carries a coarse status so observers can distinguish success from error without treating Error events as lifecycle terminators. Wire QueryManager to emit Error followed by QueryEnd(status: "error") when submit fails, while preserving QueryEnd(status: "success") for normal and cached results. Update observeLogger to correlate starts and ends by query id instead of raw SQL text, avoiding fragile matching for concurrent identical queries and ensuring failed query groups are closed. Add coverage for query ids/status, cached lifecycle events, failed query termination, and logger cleanup on query errors.
MosaicErrorEvent now carries an optional original error payload in addition to its stable message string. This keeps existing message-based consumers working while allowing diagnostic observers to retain Error objects, stack traces, and non-Error rejection values. Thread the original caught value through QueryManager, Coordinator, and PreAggregator error emissions, and update observeLogger to prefer the original error payload when available. Add focused test coverage to verify query failures preserve the Error object and logger output still closes the query group correctly.
Derive query elapsed time from QueryStart/QueryEnd event timestamps instead of observer-local wall-clock reads, keeping the event stream as the source of truth for logger rendering. Only close console groups that this observer opened for a matching query id, so unmatched QueryEnd events cannot accidentally close an unrelated console group. Clear in-flight query state on unsubscribe to avoid retaining observer-local lifecycle state. Add focused coverage for timestamp-based elapsed logging, unmatched QueryEnd handling, and unsubscribe/reobserve behavior.
Rename the internal QueryManager event bus wiring method from setEventBus to attachEventBus to better reflect that the coordinator owns the observable event bus and attaches it during construction. Guard against attaching a different bus after one has already been set, while allowing idempotent attachment of the same bus. This makes the one-time wiring contract explicit for custom QueryManager instances without introducing a separate public observation surface. Add test coverage for the attachment semantics.
| } as unknown as Connector; | ||
|
|
||
| const coord = new Coordinator(connector, { logger: null }); | ||
| const coord = new Coordinator(connector); |
Comment on lines
+133
to
142
| logger(): Logger | null; | ||
| logger(logger: Logger | null): Logger | null; | ||
| logger(logger?: Logger | null): Logger | null { | ||
| if (arguments.length) { | ||
| this._logger = logger || voidLogger(); | ||
| this.manager.logger(this._logger); | ||
| this._logger = logger ?? null; | ||
| this._unobserveLogger(); | ||
| this._unobserveLogger = observeLogger(this, this._logger); | ||
| } | ||
| return this._logger!; | ||
| return this._logger; | ||
| } |
Comment on lines
+1
to
+60
| import { Dispatch, EventCallback } from "./AsyncDispatch.js"; | ||
|
|
||
| type EventMap = Record<string, unknown>; | ||
| type EventKey<E extends EventMap> = Extract<keyof E, string>; | ||
|
|
||
| /** | ||
| * Synchronous event dispatcher that pushes pre-built events directly | ||
| * to callbacks without queueing or Promise handling. | ||
| */ | ||
| export class ObserveDispatch<E extends EventMap> extends Dispatch< | ||
| E[EventKey<E>] | ||
| > { | ||
| /** | ||
| * Add an event listener callback for the provided event type. | ||
| * @param type The event type. | ||
| * @param callback The event handler callback function to add. | ||
| */ | ||
| addEventListener<K extends EventKey<E>>( | ||
| type: K, | ||
| callback: EventCallback<E[K]>, | ||
| ): void; | ||
| override addEventListener( | ||
| type: string, | ||
| callback: EventCallback<E[EventKey<E>]>, | ||
| ): void { | ||
| super.addEventListener(type, callback); | ||
| } | ||
|
|
||
| /** | ||
| * Remove an event listener callback for the provided event type. | ||
| * @param type The event type. | ||
| * @param callback The event handler callback function to remove. | ||
| */ | ||
| removeEventListener<K extends EventKey<E>>( | ||
| type: K, | ||
| callback: EventCallback<E[K]>, | ||
| ): void; | ||
| override removeEventListener( | ||
| type: string, | ||
| callback: EventCallback<E[EventKey<E>]>, | ||
| ): void { | ||
| super.removeEventListener(type, callback); | ||
| } | ||
|
|
||
| /** | ||
| * Emit an already-constructed event value to listeners for the given event type. | ||
| * @param type The event type. | ||
| * @param value The complete event object. | ||
| */ | ||
| emit<K extends EventKey<E>>(type: K, value: E[K]): void; | ||
| override emit(type: string, value: E[EventKey<E>]): void { | ||
| const callbacks = this._callbacks.get(type); | ||
| if (!callbacks?.size) return; | ||
|
|
||
| const event = this.willEmit(type, value); | ||
| for (const callback of callbacks) { | ||
| callback(event); | ||
| } | ||
| } | ||
| } |
Comment on lines
+35
to
+45
| const openedGroup = t0 != null; | ||
| starts.delete(event.queryId); | ||
| const elapsed = t0 == null ? undefined : (event.timestamp - t0).toFixed(1); | ||
|
|
||
| if (elapsed != null) { | ||
| logger.log(event.query, elapsed); | ||
| } else { | ||
| logger.log(event.query); | ||
| } | ||
|
|
||
| if (openedGroup) logger.groupEnd(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This continues the devtools event bus work originally started by @theVedanta in #950.
The branch has been rebased onto the current
uwdata/mosaicmainbranch to resolve merge conflicts and bring the work up to date with recent upstream changes.I’ve also addressed follow-up items around the implementation and tests so this can move toward review and merging.
Original branch:
https://github.com/theVedanta/mosaic/tree/devtools-event-bus