radicle-dev / radicle-upstream

Desktop client for Radicle.
Other
615 stars 51 forks source link

Replace Svelte store in event handling code #1800

Closed geigerzaehler closed 3 years ago

geigerzaehler commented 3 years ago

At the moment we are using Svelte stores and in particular svelteStore.derived to create objects that allow us to listen to events from the proxy (example). Because we use stores we are already seeing the following problems

First, we need to be careful when subscribing to an “event” store. The subscriber callback is called with the last event when the subscription is created—even if that event occurred ages ago. Second, writing filters to select a subset of events requires us to use null as a placeholder for the absence of events which leads to bloated and brittle code.

The root cause is that a Svelte store does not exhibit the behavior we want. It always has a current value. Instead we want a stream.

To address the issue I suggest we use a reactive programming library that provides first class reactive primitives including streams. For TypeScript the most widely used candidates are RxJS and BaconJS. While BaconJS is used less, it is also smaller, provides a more concise (and less Java-like) API and has better documentation. I suggest we adopt it.

cloudhead commented 3 years ago

I'm curious why we use stores that way currently, the example you link (ProjectUpdated) seems like an abuse of the store concept. Instead we should have (in this case) a ProjectState type that can be updated with new state. The app can then always show the latest state. This gets around the problem of "null" events. Basically svelte is already functional-reactive.

Do you have examples of where an event emitter/stream is actually what we want?

cloudhead commented 3 years ago

Reflecting on this more after a chat with xla, I guess the place where you envision this fitting is the ??? in:

proxy -> sse events -> ??? -> state stores -> ui

Where ??? is currently the "event store".

Is that right?

geigerzaehler commented 3 years ago

proxy -> sse events -> ??? -> state stores -> ui

Where ??? is currently the "event store".

Is that right?

Yes, that’s correct.

Instead we should have (in this case) a ProjectState type that can be updated with new state.

If I understand this correctly this would invert the dependencies compared to the current approach. At the moment we have code in src/Screen/Project.svelte that listens for project events and updates the store https://github.com/radicle-dev/radicle-upstream/blob/a8cf264bd77e8dc84b702484ab445006b12decbe/ui/Screen/Project.svelte#L47-L56

Here the UI code decides which events to listen to (filtering) and updates the store accordingly.

In your approach the event code would decide when to update which store. This would split the filtering logic from the store logic which I don’t think is a good idea. Moreover, the event module would now get a dependency on every state store that it would need to update.

Because of these down-sides I think a pull approach is more appropriate.

geigerzaehler commented 3 years ago

I’ve created a PoC for this to shed some more light on the details: https://github.com/radicle-dev/radicle-upstream/pull/1805

cloudhead commented 3 years ago

Thanks for clarifying!

If I understand this correctly this would invert the dependencies compared to the current approach.

Yes this is correct. Instead of having event logic in the UI code, the UI code simply renders the state of the stores.

The stores then listen on events of interest and update themselves when needed. Hence stores store state (ProjectState), not events (ProjectUpdated).

Moreover, the event module would now get a dependency on every state store that it would need to update.

This is simple to decouple: every entity subscribes to the event feed and updates its store (outside of the UI code).

geigerzaehler commented 3 years ago

The stores then listen on events of interest and update themselves when needed. Hence stores store state (ProjectState), not events (ProjectUpdated). This is simple to decouple: every entity subscribes to the event feed and updates its store (outside of the UI code).

Yes, that’s what I was suggesting, too. In my description I was focusing on the “subscribes” part. What we subscribe to should be an event stream that allows for easy filtering and not a svelte store. (Basically we want the same behavior as a Rust futures Stream.) Can you check if the PoC is in line with what you’re expecting there? It doesn’t consolidate the subscription code yet but it provides a good handle to do this more effectively.

cloudhead commented 3 years ago

Cool, it looks like an improvement, but left a comment on the part that still isn't clicking for me.

geigerzaehler commented 3 years ago

Fixed in #1805