Subscriptions: Allow events to be skipped with broadcastable

[vaot]

Is your feature request related to a problem? Please describe.

Currently, the one way I can see to skip updates is to set broadcastable to false as documented here. This change, in turn, means that the payload will be computed for every single subscriber. Oftentimes, the skipping criteria does not depend on the payload, rather it depends on some context parameters. In these cases, computing the payload for every subscriber adds overhead.

Describe the solution you'd like

Skip updates(events) with broadcastable set to true. The payload would still be computed once, but the skipping criteria would depend on the context and the one computed result.

A quick tweak that I came up with is to add the following line

next if @schema.respond_to?(:skip_event?) && @schema.skip_event?(object, event.context)

to this block.

Describe alternatives you've considered

The other alternative is to just use the current mechanism and set broadcastable to false and use def update to skip updates.

Or is there any other way to accomplish this right now that would not require a change ?

Additional context

Say, there is a list of items. On the client side, there are a bunch of filters that allows users to filter the list. These filters are passed in as parameters to the channel. Updates sent to the subscribers of the same list should take into account the filtering of each subscriber. Note that this only requires the payload to be computed once, and the filtering can be done after the first payload has been computed.

[rmosolgo]

👋 Hi, thanks for the suggestion. I'd like to understand your use case a little more. It sounds like each client is listening to stream of objects, but the clients may apply different filters to that stream, is that right? For example:

subscription($handle: String!, $pattern, String!) {
  tweetWasTweeted(from: $handle, matches: $pattern) {
    body 
  }
}

Well, an example like that ☝️ won't actually work very well, because you'd have to .trigger with a pattern that matched the user-provided string. Is that why you pass the filters as "parameters to the channel"? (Or did you mean something else by that?)

---

Ok, maybe I'm starting to see how you implemented it, that's interesting! You have an unfiltered subscription like:

subscription {
  tweetWasTweeted { body } 
}

And, using context, you can know whether a certain subscriber actually wants to receive a new tweet or not. You'd like to:

• Generate the payload only once, since it's broadcastable;
• But, skip sending it in some cases, since you can identify clients that don't actually want it.

def update isn't good enough, because it's not called for each subscriber.

How's my understanding of the situation?

If I've got it right, I'm inclined to add this as something like def self.skip_update?:

class TweetWasTweeted < Subscriptions::Base 
  # If this method returns false, don't update the subscriber for `ctx`
  def self.skip_update?(obj, ctx)
    !ctx[:filter].match?(obj) 
  end 
end 

Somehow, that'd have to fit in with (by taking priority over) the current def update API.

What do you think of it?

[vaot]

@rmosolgo Thank you for considering it. Yeah, your second explanation is correct. We have an unfiltered subscription. And yes, we want to leverage properties of the context to skip the update. We have a context that looks something like { current_user: ..., filters: ... }. These filters decide which updates should be skipped for a given client.

def update isn't good enough, because it's not called for each subscriber.

That's correct.

Let's take your example subscription:

subscription {
  tweetWasTweeted { body } 
}

Say, you are searching for tweets whose body has the word awesome. In your view, now, you are seeing tweets with the word awesome, any updates for tweets that do not match this criteria would be skipped. The search query is one of the filters we are passing into the context. This would be one scenario.

And yes, I think self.skip_update? would be even better, since it wouldnt leak into the general schema like I suggested. Would we need to fit this new method here ?

[rmosolgo]

Sorry I never got back to this -- as it stands, I don't plan to commit any time to it so I'm going to close it out. If anyone wants to pursue a specific implementation, I'd be happy to review a PR or discuss it further in another issue :+1: