feat: Devtools - A separate, lightweight event bus -- utilizing Dispatch (Sync)#950
feat: Devtools - A separate, lightweight event bus -- utilizing Dispatch (Sync)#950theVedanta wants to merge 11 commits intouwdata:mainfrom
Conversation
domoritz
left a comment
domoritz
left a comment
There was a problem hiding this comment.
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.
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.
do we need this override? Looks equivalent
There was a problem hiding this comment.
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.
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.
as any is worrying me a bit here
There was a problem hiding this comment.
I'm thinking we get rid of the clientId for now?
theVedanta
changed the title
| output.replaceChildren(); | ||
| } else { | ||
| await coordinator().exec( | ||
| await vg.coordinator().exec( |
There was a problem hiding this comment.
honestly, I'm not really sure. this example was broken when I tested it; the coordinator is not accessible as an object here, but only as a property of vg
There was a problem hiding this comment.
Ahh, good catch. I'll fix it separately. Let's keep PRs focused on one thing.
| enable(); | ||
| }); | ||
|
|
||
| function setQueryLog() { |
There was a problem hiding this comment.
Shouldn't this logic (which replicates the old logger, right?) be in mosaic core so that people can easily get the old logger behavior again?
There was a problem hiding this comment.
ahh. we can definitely do that. My mistake, I felt that when we said 'remove the logger from core entirely,' we give the client the option to write that logic. I can put this function in core to give back a logger util to the user/client (perhaps we can discuss that idea some more (?))
There was a problem hiding this comment.
IIRC the idea was to remove the logger but give users an equivalent tool via the new bus.
| vg.coordinator().manager.logQueries(qlogToggle.checked); | ||
| const bus = vg.coordinator().eventBus; | ||
|
|
||
| bus.observe("QueryStart", (event) => |
| if (cached) { | ||
| const data = await cached; | ||
| this._logger.debug('Cache'); | ||
| const data = cached; |
There was a problem hiding this comment.
Do we not need to await anymore?
There was a problem hiding this comment.
await will have no effect here
There was a problem hiding this comment.
Let's make a separate pull request. IIRC there was some reason where cached results can be promises but I am not 100% sure right now.
| this.eventBus?.emit(EventType.Error, { | ||
| message: err, | ||
| }); |
There was a problem hiding this comment.
Is this strictly needed to replicate the behavior of the old logger?
There was a problem hiding this comment.
Well, all it's doing it just pipe whatever the message was (which logger emitted) into the bus, which is now consumable by anyone, including the logger. I don't know if Im missing something, did you have a simpler approach in mind?
There was a problem hiding this comment.
I just didn't see a log message (we removed) here before so I am wondering whether the emit is needed.
| Error = "error", | ||
| } | ||
|
|
||
| export type EventMap = { |
There was a problem hiding this comment.
I've never seen this pattern before. Why is this needed?
There was a problem hiding this comment.
This was mainly for type-safety (for us and the clients)
There was a problem hiding this comment.
Let's not do this and instead use the class approach I suggested?
| QueryStart = "query-start", | ||
| QueryEnd = "query-end", | ||
| ClientConnect = "client-connect", | ||
| ClientStateChange = "client-state-change", |
There was a problem hiding this comment.
I don't see client-state-change used anywhere.
| @@ -0,0 +1,43 @@ | |||
| export enum EventType { | |||
There was a problem hiding this comment.
Instead of having this enum here. Wouldn't it be easier to have classes for the events that can be instantiated with a constructor?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
2 participants
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