vaadin / collaboration-engine

The simplest way to build real-time collaboration into web apps
https://vaadin.com/collaboration
Other
3 stars 1 forks source link

Provide a low-level API for sending events/transient messages #34

Open emarc opened 3 years ago

emarc commented 3 years ago

Is your feature request related to a use case? Please describe. Need to send transient values, like events or messages, e.g to notify a user when something they are subscribed to changes, or for signalling in a WebRTC solution.

Describe the solution you'd like

           CollaborationEventBus eventBus = topic.getEventBus("station-123");
            eventBus.subscribe(event -> { 
                switch (event.getType()) {
                    // EventBus could behave like a CollaborationMap with transient values
                    // = multiple event types per bus
                }
                // OR just emit one event type per EventBus
                showNotification(event.getDetail(String.class));
            });
            // N/A: eventBus.get(...)

Describe alternatives you've considered A mode for all CollaborationX data structures to have a no-history mode, so that each subscriber would only see values changed since they subscribed. This would allow making a chat where late joiners do not see the chat history (this is the way that e.g Google Docs chat works). This would not be a perfectly natural way to implement and EventBus, but pretty close. These cases should maybe be split into two?

Additional context One specific use-case where it could be used: https://stackoverflow.com/questions/67385825/best-practice-push-notifications-in-vaadin-and-spring-boot

heruan commented 3 years ago

Thanks for the feedback! There has been some internal discussion about a CollaborationChannel to handle this kind of use case. Or, this may also be implemented as a subscription option to a CollaborationList, for example:

CollaborationList notifications = topic.getNamedList("notifications");
notifications.subscribe(event -> {}); // <- this will receive missed notifications
notifications.subscribeOnlyNew(event -> {}); // <- this will receive new notifications only

On the other hand, a WebRTC solution would benefit from a channel/bus data structure, without history.

Legioth commented 3 years ago

As a workaround, you could use something like this to emulate the functionality by putting and immediately removing a value in a CollaborationMap.

public class EventChannel<T> {
  private final CollaborationMap map;
  private final String key;
  public EventChannel(Class<T> eventType, TopicConnection connection, Consumer<T> listener) {
    map = connection.getNamedMap(EventChannel.class.getName());
    key = eventType.getName();
    map.subscribe(event -> {
      if (!key.equals(event.getKey()) return;
      T value = event.getValue(eventType);
      if (value != null) listener.accept(value);
    });
  }
  public void submit(T event) {
    map.put(key, event);
    map.put(key, null);
  }
}
Legioth commented 3 years ago

It might make sense to implement this as a "middle-level" API, i.e. a DAO similar to the ones described in #36. This approach would allow us to provide a high-level API in a logical location without having to add more complexity to the core. Also, usage would be easier since you wouldn't need two nested lambdas with one for the connection callback and another one for a listener. One drawback is that discoverability might be better if there's an instance method on TopicConnection, but that still assumes that someone has already figured out TopicConnection. Another small drawback is that this approach would most likely (ab)use CollaborationMap for the low-level synchronization, which might in turn cause naming conflicts. Right now, it doesn't feel like either potential drawback would be significant enough to consider some other alternative.

One specific thing that needs to be taken into account in the API design is to help the user deal with the overlap when starting to listen for events. This is useful in cases such as when using an event to notify that some data has been changed so that e.g. a Grid can be refreshed. Without any special mechanism, one couldn't know when to do an initial refresh to be sure that nothing is missed since we cannot guarantee that subscribing is synchronous. Instead, we should either fire an initial event to confirm the subscription or alternatively use a separate event type for this purpose. One API for doing this without too much complexity would be to have two overloads of the method for adding a subscriber - one that only takes a subscriber and another one that takes both a subscriber and an event object that should be fired only to this particular listener instance when the subscription becomes active (which might happen multiple times if the connection context is deactivated and then activated again).

Based on these ideas, this whole thing could be used to fire and listen to events to update rows in a grid like this:

// DAO instance shared on the UI instance level
gridRefreshDao = new EventCollaborationDao<>(MyItem.class, grid, "topicId", localUserInfo);

// Notifying about changes for a specific item
gridRefreshDao.fireEvent(changedItem);

// Notifying about changes for the whole grid (i.e. after adding or removing items)
gridRefreshDao.fireEvent(null);

// Listening for changes. The null parameter is the event to fire when subscription is activated, thus triggering an initial refresh
gridRefreshDao.onEvent(null, event -> {
  if (event == null) grid.getDataProvider().refreshAll();
  else grid.getDataProvider().refreshItem(event);
});

Similarly, to show notifications to all active users of the application, something like this could be used.

// DAO instance shared on the UI instance level
globalNotificationDao = new EventCollaborationDao<>(String.class, UI.getCurrent(), "topicId", localUserInfo);

// Publishing a notification
globalNotificationDao.fireEvent("System maintenance starting in 30 minutes");

// Receiving notifications
globalNotificationDao.onEvent(Notification::show);
Legioth commented 3 years ago

The implementation could use the same approach as in my previous comment with a double put in a CollaborationMap. The only difference is that we would have to use a placeholder value instead of null if we want to support using null as an event (like in the case with refreshAll()). We would also need to use another placeholder value to signal the baseline so that new subscribers would get an initial poke that they can use to fire a local event with the initial event value used when registering the subscriber.

pekam commented 3 years ago

In general, the DAO approach sounds good to me! It provides API that is more suitable for the use cases without polluting the core API with something that would technically just be a wrapper for the CollaborationMap primitive.

One detail I'm not sure about is the API to react to activated connection by passing the event object (or null) as an argument to onEvent:

gridRefreshDao.onEvent(null, event -> {
  if (event == null) grid.getDataProvider().refreshAll();
  else grid.getDataProvider().refreshItem(event);
});

It doesn't feel very intuitive to me.

A bit more verbose approach would be to have another method to react to connection activation:

gridRefreshDao.onEvent(event -> {
  if (event == null) grid.getDataProvider().refreshAll();
  else grid.getDataProvider().refreshItem(event);
});

gridRefreshDao.onActivation(() -> {
  grid.getDataProvider().refreshAll();
});

In this case it needs to repeat one line of code, because null event was also used in this example to notify when items are added/removed, but now it's possible to understand what the code does by just reading it.

Furthermore, this onActivation method could be defined in a common super class/interface of all the DAOs, since all of them open a topic connection under the hood. This might enable more use cases which we're not yet aware of.

Legioth commented 3 years ago

While I agree that something like onActivation should be available on all DAO classes, I don't think it helps to fully solve this particular case. The reason is that you might end up doing a double refreshAll() in case an event is fired between the times when onEvent and onActivation are called. If you try to compensate by reversing the order, then you might instead miss an event. For that reason, both aspects must be within a single "transaction" to be absolutely sure they are in sync. (Another alternative would be that we make the internals more complicated so that everything registered with CE within the same UI.access block are treated as the same transaction.)

It's then a different question whether this case is important enough to make the API more complex. There's nothing that prevents us from starting out with only the simpler approach and then adding something more complex later on when and if there is a specific demand for it.

Legioth commented 2 years ago

One additional thing to consider here is how to relate to temporarily being "disconnected". Depending on the use case, it might be problematic if someone misses an event because they happened to reload their browser tab at the same moment when an event occurred.

I was initially thinking that this could be handled by giving each entry in a list or map a custom life time so that it would be automatically deleted at a predefined time. This would on the other hand be slightly impractical for users since they would then still receive deletion events that they need to ignore.