feat: filter subscription API

[danisharora099]

Problem

The current Filter implementation has a few gaps:

Dev Ex

• library consumers are more used to an event-based approach for subscription-like implementations
  • await filter.subscribe(xyz, callback) works can be improved as it unintuitive to use await in a scenario like such
  • filter.subscribe.addEventListener(xyz) feels more intuitive

Readability and Maintainability

• the implementation has logic like

  js-waku/packages/core/src/lib/filter/index.ts

  Line 417 in 1ec0c20

  const decodedMessage = await Promise.any(decodePromises);

  and

  js-waku/packages/core/src/lib/filter/index.ts

  Line 151 in 1ec0c20

  this.subscriptionCallbacks.set(contentTopic, subscriptionCallback);

  that can be improved where we store the decoder, per content topic, and don't need to store callbacks and are able to dispatch the decoded message

Proposed Solutions

API:

  const subscription = await waku.filter.createSubscription(decoders);
  
  subscription.addEventListener(
        createKeyFromDecoder(decoder),
        (evt) => {
         const decodedMessage = evt.detail;
         }
      );

which users are more familiar with.

Along with that, improvements like:

• separate Filter and Subscription files
• takes in decoders on Filter.createSubscription() and calls it internally to avoid an extra step
• use event emitters for Subscription

Notes

• PR addressing this issue: chore: refactor Filter to an event-based API #1682
• related to bug: align filter subscribe to relay behavior #1678 and dev ex

[fryorcraken]

Maybe await is unuintitive but the event listener is not setup until the promise is resolved and the network interaction is done. So not sure we can hide that away from a developer that may think that the event listener is setup as soon as the call is done.

an event listener usually has the following signature:

addEventListener(eventType: string, callback: (event: Event) => void)

but here, we are need to have the decoder instead of a string. To me this feels even less intuitive.

[fryorcraken]

Maybe it's should be a two step process?

// get event listener ready
filter.addEventListener("new-message", (msg) => {...})

// Subscribe to messages
await filter.subscribe(decoder)
..

[vpavlin]

@danisharora099 Looking at this in more detail now

In your proposed API, are you actually subscribing during the createSubscription call? I.e. the async operation still has the await, but then the addEventListener call is there just to setup the actual action for when a new message is received?

If that is the case, then it sounds good to me, as that would match the approach I took in dispatcher:

1. Subscribe during initialization
2. Then setup the callback(s) later - see the actual usage here

That said, dispatcher is higher level abstraction, so I have many more event types, but I could still see js-waku Filter API have events like:

• new-message
• ping-failure
• unsubscribed
• subscribed

On the other hand, if the flow is

1. createSubscription
2. addEventListener
3. subscribe

Then it may not really help the devexp

[danisharora099]

@vpavlin

On the other hand, if the flow is

1. createSubscription
2. addEventListener
3. subscribe

Then it may not really help the devexp

The flow is simply:

1. filter.createSubscription()
2. addEventListener

That's it. No subscribe() call necessary.

---

@fryorcraken

but here, we are need to have the decoder instead of a string. To me this feels even less intuitive.

we're not actually passing the decoder, but creating a unique key identifier (which is a string) using the decoder. The way key gen happens is by concatenating the pubsubTopic_contentTopic. I'll look into how we can improve this, especially since each Subscription object is specific to one pubsub topic, so just passing the contentTopic as key should be enough :)

[fryorcraken]

@fryorcraken

but here, we are need to have the decoder instead of a string. To me this feels even less intuitive.

we're not actually passing the decoder, but creating a unique key identifier (which is a string) using the decoder. The way key gen happens is by concatenating the pubsubTopic_contentTopic. I'll look into how we can improve this, especially since each Subscription object is specific to one pubsub topic, so just passing the contentTopic as key should be enough :)

With autosharding only the content topic would be needed. Hence, I suggest to only pass the content topic to addEventListener:

function addEventListener(contentTopic: string, (event: Event) => void): void

Exception should be thrown if there are no active or pending subscription for said content topic to help developer ensure they use the API right.

There is one corner case where a developer uses the same content topic on 2 different static shard and wants different event processing. In this case, the API does not allow the developer to register different callback.

I think it's a rare scenario and can easily be mitigated by having the developer call addEventListener twice for each processing and do a shard check at the start of the callback.

[weboko]

Independently from this discussion I implemented addEventListener for REST API here

Takes I have so far (some of which are relevant to existing API too):

• not possible to handle error if subscribe request fails;
• not possible to expose ping and other API if we want to keep API concise and minimal (I guess things like re-subscribe, ping etc should be done inside of js-waku and exposed to advanced users through other API);

Also points from previous posts are valid.

And to confirm other opinions in this thread - I don't see anything bad in addEventListener API but it should be the same as in the browser, e.g follow this declaration:
addEventListener(contentTopic: string, (event: CustomEvent) => void): void

[danisharora099]

With autosharding only the content topic would be needed. Hence, I suggest to only pass the content topic to addEventListener:

function addEventListener(contentTopic: string, (event: Event) => void): void
Exception should be thrown if there are no active or pending subscription for said content topic to help developer ensure they use the API right.

I agree with your point and we can easily remove the overhead of managing pubsub topics for key, especially since each Subscription object represents a unique pubsub topic.

addressed this architecture here: 1e732a9

[fryorcraken]

• not possible to handle error if subscribe request fails;

This would be handled with createSubscription throwing/rejecting, no?

[danisharora099]

• not possible to handle error if subscribe request fails;

This would be handled with createSubscription throwing/rejecting, no?

yes

[weboko]

Yes, for the subscribe request but not invocation - wrong wording.
These places are hard to reach and if something happens - user has no way to react to error: here and here and maybe other places.

I see how we can handle - is to expose error event to Emit API or we can experiment and try to combine events with Node.js approach:

type CustomEvent = Event & {
  detail: {
    payload?: any,
    error?: any,
  }
};
filter.addEventListener("/content/topic", (event: CustomEvent) => {
  ...
});

[chair28980]

Notes from js-waku pm 2023-11-07

• Danish
  • https://github.com/orgs/waku-org/projects/2/views/1?pane=issue&itemId=42532971
  • Will be a significant change. Currently we store callbacks in the class implementation
  • We want to switch to an easier API for devex. We will emit events for the filter subscription
  • We are on the same page about what the api should look like, will tackle once autosharding is done
  • With the filter refactor, we currently have an interface that 2 classes are compliant with, i-receiver. Changing just filter breaks our compliance, requires also refactoring relay.
  • Issue will breakdown to 2 prs, change filter, and refactor relay
• Sasha
  • Could be good task for Arseniy
• Danish
  • First pr will be merged without complance with i-reciever, if we want compliance for both we should do it in 1 pr.
• Sasha
  • Not necessary for us to be compliant, should be released in one go
• Danish
  • Temporarily filter implementation is not going to be compliant with i-receiver until we merge the relay api

[danisharora099]

tracking followup as discussed in PM meeting here: #1713

[fryorcraken]

waku-org/docs.waku.org#132 (comment)

ping API can also be improved

[danisharora099]

Weekly update:

• achieved: PR opened and currently under review-iteration phase ( feat!: return response interface for success/error #1725 )
• next: merge PR

[danisharora099]

Weekly update:

• achieved: rebasing
• next: error handling via event emitter as well. plan if restructuring is required.

[weboko]

Moved to Triage till 11th of Jan.

To discuss by team:

• how to release work in a non breaking way;
• how to distribute tasks in a team;

What work is ongoing:

• Filter update;
• Relay update;
• migration of an example to new API;

cc @waku-org/js-waku-developers

[fryorcraken]

I would recommend to keep the filter API as close as possible to the protocol. Even if it means the dev ex is not the best,

Instead, I would focus on waku-org/pm#114 and from there, provide the best API to the user.

This API can then be simple and abstract a lot:

API to dev: pass callback and content topic

In the background:

• build decoder
• connects to several filter nodes
• ping filter node regularly
• disconnect to unresponsive nodes
• handle subscription creation and subscribe call
• use store protocol regularly/when all filter were disconnected and then reconnected
• etc

[weboko]

@danisharora099 removing this feat from reliability milestone