-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
base: main
Are you sure you want to change the base?
Conversation
domoritz
left a comment
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.
Very cool start. looking good. Here are some comments.
dev/index.html
Outdated
| function setQueryLog() { | ||
| vg.coordinator().manager.logQueries(qlogToggle.checked); | ||
| // logQueries is no longer in use | ||
| // vg.coordinator().manager.logQueries(qlogToggle.checked); |
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.
| * callback function to remove. | ||
| */ | ||
| override removeEventListener(type: string, callback: EventCallback<T>): void { | ||
| super.removeEventListener(type, callback); |
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 we need this override? Looks equivalent
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.
I think I had some extensibility in mind for later, but on at another glance, I think this feels unnecessary. Removing...
| if (arguments.length) { | ||
| this._logger = logger || voidLogger(); | ||
| this.manager.logger(this._logger); | ||
| // subscribe logger to events |
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.
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.
| ): Promise<unknown> { | ||
| client.queryPending(); | ||
| return client._pending = this.query(query, { priority }) | ||
| return client._pending = this.query(query, { priority, clientId: (client as any).id }) |
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.
as any is worrying me a bit here
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.
I'm thinking we get rid of the clientId for now?
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.
Let's do that
| .then( | ||
| data => client.queryResult(data).update(), | ||
| err => { this._logger?.error(err); client.queryError(err); } | ||
| err => { this.eventBus.emit(EventType.Error, { message: err, timestamp: Date.now() }); client.queryError(err); } |
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.
I wonder whether we really need timestamps since the event bus could maybe automatically add it for all emitted events? Could make the callsites simpler.
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.
absolutely, lemme go ahead and do that
packages/mosaic/core/src/EventBus.ts
Outdated
| timestamp: number; | ||
| } | ||
|
|
||
| export class EventBus { |
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.
Why do we need this class? It seems to just be dispatching events to the ObservableDispatch anyway. If we make the latter generic, we may not need this class at all.
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.
I wondered about just using ObservableDispatch too, but I feel that a separate EventBus would actually be extensible in the future for various reasons like maintaining a history of events between renders (for frameworks like react/preact/solid/etc). That said, if you would still like for me to just use ObserveDispatch directly, to keep things light, we can definitely do that
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.
Let's keep things light.
| const cached = this.clientCache!.get(sql!); | ||
| if (cached) { | ||
| const data = await cached; | ||
| this._logger.debug('Cache'); |
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.
Let's remove the logger and replace the same location with the event bus stuff.
| this._logger.debug('Cache'); | ||
| result.ready(data); | ||
| // emit QueryEnd for cached | ||
| if (this.eventBus) { |
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.
It's odd to me that this is after result.ready(data) when previously the logging was happening before.
| this._logger.debug(`Request: ${(performance.now() - t0).toFixed(1)}`); | ||
| result.ready(type === 'exec' ? null : data); | ||
|
|
||
| if (this.eventBus) { |
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.
same as above
| }); | ||
| } | ||
| } catch (err) { | ||
| if (this.eventBus) { |
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.
Is this needed to replicate the logic of the logger? If not, let's not add it yet.
|
|
||
| if (this.eventBus) { | ||
| this.eventBus.emit(EventType.QueryEnd, { | ||
| query: sql || '', |
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.
If we have an undefined sql, we can just output that.
| public filterGroups = new Map<Selection, FilterGroupEntry>; | ||
| protected _logger: Logger = voidLogger(); | ||
| public eventBus: EventBus; | ||
| public eventBus: ObserveDispatch<any>; // temporary measures until we finalize event types |
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.
Let's use the types we already have. We can expand it later.
|
|
||
| export interface MosaicEvent { | ||
| timestamp: number; | ||
| // Extend later with more fields |
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.
No need to say that here.
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.
the extend later part?
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.
Yeah, the whole comment.
packages/mosaic/core/src/Events.ts
Outdated
| } | ||
|
|
||
| export interface ErrorEvent extends MosaicEvent { | ||
| message: unknown; |
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.
Should it be unknown or string?
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.
string for sure; I don't remember why I typed this as unknown
.prettierrc.json
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.
let's do in the other pull request
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.
oh sorry i was just using this locally, my bad
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.
All good. Let me know when you addressed the comments above. I think we are getting close.
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.
Done :) Anything else I may have missed?
This commit resolves some tasks discussed in the meeting with @domoritz. In response to the issue #943
For an overview, these are some of the immediate action items discussed:
Action items
.observemethod to a single place on the coordinatorobserver,Setof event types (not provided subscribes to all events)logQueriesbool from coordinatorDispatchAsyncDispatchextends itObserveDispatchalso extends it.observeroutes to observe dispatch.addEventListenerThe UI-related example, written in Solid JS, will be in a separate PR. The current goal is to switch out the logger in Coordinator and QueryManager to consume our event bus for uncertain event types (which will be crystallized later for devtools).
https://docs.google.com/document/d/1cyw2sv9G5-CzGG-JgeOEcISSu7zmEqdzsa-J2oh2F6c/edit?tab=t.0