-
-
Notifications
You must be signed in to change notification settings - Fork 117
feat: Devtools - A separate, lightweight event bus -- utilizing Dispatch (Sync) #950
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
Changes from 8 commits
859468e
1cfd7ef
5fe505c
8dcba1c
0f8cec0
8a7b56e
3cd95e0
be0b792
353fcaa
a5990bb
d37a487
cd9a4e2
75153b1
820a8cc
b6999f7
c8a2c3f
d89ca14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ import { type MosaicClient } from './MosaicClient.js'; | |
| import { type SelectionClause } from './SelectionClause.js'; | ||
| import { MaybeArray } from '@uwdata/mosaic-sql'; | ||
| import { Table } from '@uwdata/flechette'; | ||
| import { EventType } from './Events.js'; | ||
| import { ObserveDispatch } from './util/ObserveDispatch.js'; | ||
|
|
||
| interface FilterGroupEntry { | ||
| selection: Selection; | ||
|
|
@@ -50,6 +52,7 @@ export class Coordinator { | |
| public clients = new Set<MosaicClient>; | ||
| public filterGroups = new Map<Selection, FilterGroupEntry>; | ||
| protected _logger: Logger = voidLogger(); | ||
| public eventBus: ObserveDispatch<any>; // temporary measures until we finalize event types | ||
|
domoritz marked this conversation as resolved.
Outdated
|
||
|
|
||
| /** | ||
| * @param db Database connector. Defaults to a web socket connection. | ||
|
|
@@ -77,7 +80,9 @@ export class Coordinator { | |
| consolidate = true, | ||
| preagg = {} | ||
| } = options; | ||
| this.eventBus = new ObserveDispatch(); | ||
| this.manager = manager; | ||
| this.manager.eventBus = this.eventBus; | ||
| this.manager.cache(cache); | ||
| this.manager.consolidate(consolidate); | ||
| this.databaseConnector(db); | ||
|
|
@@ -126,6 +131,12 @@ export class Coordinator { | |
| if (arguments.length) { | ||
| this._logger = logger || voidLogger(); | ||
| this.manager.logger(this._logger); | ||
|
|
||
| // subscribe logger to events | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good as a stepping stone but I think we can go ahead and make the logger a separate observer that users need to explicitly add. We can remove the logger entirely now imo. |
||
| this.eventBus.observe(EventType.QueryStart, (event) => this._logger.info('Query started:', event)); | ||
| this.eventBus.observe(EventType.QueryEnd, (event) => this._logger.info('Query ended:', event)); | ||
| this.eventBus.observe(EventType.ClientConnect, (event) => this._logger.info('Client connected:', event)); | ||
| this.eventBus.observe(EventType.Error, (event) => this._logger.error('Error:', event.message)); | ||
| } | ||
| return this._logger!; | ||
| } | ||
|
|
@@ -249,9 +260,9 @@ export class Coordinator { | |
| return client._pending = this.query(query, { priority }) | ||
| .then( | ||
| data => client.queryResult(data).update(), | ||
| err => { this._logger?.error(err); client.queryError(err); } | ||
| err => { this.eventBus.emit(EventType.Error, { message: err }); client.queryError(err); } | ||
| ) | ||
| .catch(err => this._logger?.error(err)); | ||
| .catch(err => { this.eventBus.emit(EventType.Error, { message: err }); }); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -280,6 +291,11 @@ export class Coordinator { | |
| throw new Error('Client already connected.'); | ||
| } | ||
|
|
||
| // emit ClientConnect | ||
| this.eventBus.emit(EventType.ClientConnect, { | ||
| // additional arguments for later -- for comprehensive client data | ||
|
theVedanta marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
|
||
| // add client to client set | ||
| clients?.add(client); | ||
|
|
||
|
|
@@ -394,4 +410,4 @@ function updateSelection( | |
| const query = info?.query(active.predicate) ?? client.query(filter); | ||
| return mc.updateClient(client, query); | ||
| })); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| export enum EventType { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of having this enum here. Wouldn't it be easier to have classes for the events that can be instantiated with a constructor?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is probably true. It will get rid of this "broken"-ish type interface for the events. But I remember that we agreed on enums (denoted by strings instead of numbers) to provide the user a simple list of event types in autocomplete
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we can still use enums in the event types but in order to create an event object, we could use a class. Maybe it's cleaner to actually put the event type into the message so that we can easily type check a message in isolation. Let's do that. |
||
| QueryStart = 'query-start', | ||
| QueryEnd = 'query-end', | ||
| ClientConnect = 'client-connect', | ||
| ClientStateChange = 'client-state-change', | ||
| Error = 'error' | ||
| } | ||
|
|
||
| export interface MosaicEvent { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be private/not exported?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like we'll need this type when omitting pre-set keys in the coordinator/query-manager |
||
| timestamp: number; | ||
| // Extend later with more fields | ||
|
domoritz marked this conversation as resolved.
Outdated
theVedanta marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| export interface QueryStartEvent extends MosaicEvent { | ||
| query: string; | ||
| materialized: boolean; | ||
| } | ||
|
|
||
| export interface QueryEndEvent extends MosaicEvent { | ||
| query: string; | ||
| materialized: boolean; | ||
| } | ||
|
|
||
| export interface ClientConnectEvent extends MosaicEvent { | ||
| clientId?: string; | ||
| } | ||
|
|
||
| export interface ErrorEvent extends MosaicEvent { | ||
| message: unknown; | ||
|
domoritz marked this conversation as resolved.
Outdated
|
||
| } | ||
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.
This is where we should add or remove our custom observer that redirects events to console.