finos / FDC3

An open standard for the financial desktop.
https://fdc3.finos.org
Other
198 stars 119 forks source link

Clarify: What should happen on repeated listener addition for the same contextType/intent #1390

Open kemerava opened 1 week ago

kemerava commented 1 week ago

Question Area

Question

What should happen in the situation where for example app does: await fdc3.addContextListener('fdc3.contact', (contact, metadata) => {... }); await fdc3.addContextListener('fdc3.contact', contact => { ... }); Or some different handler?

I see 3 possible options (in no particular order):

  1. Override the previous handler and keep the last one
  2. Keep and trigger both handlers (potential conflicts?)
  3. Throw an error and keep the original

Does the standard have any ruling on this, I was not able to find on the docs (please, let me know if I missed it)?

kriswest commented 1 week ago

The answer to this is not explicitly stated in the standard to my knowledge (it should be!). However, it consistently says that addContextListener adds a listener - overwriting or replacing an existing listener is never mentioned. Instead, a specific mechanism is provided to remove a listener (Listener.unsubscribe).

Hence, I would go ahead and assume that you can add multiple listeners of the same or overlapping types (i.e. null and a named type) and that all should trigger when a relevant context type is broadcast on the channel or current user channel as appropriate.

I think the answer is different for intent handling, where it only makes sense for a single handler to be present per intent name (context type is not an argument to addIntentListener so a single intent handler should handle all context types accepted) - particularly as only a single handler can relate to the getResult() promis, (which should resolve when the handler has run and has resolved or rejected its promise. However, there is no specified error to return in this case (ResolveError is the most relevant union but doesn't have an error relating to trying to register an intent listener).

Given these two very similar API calls are (and IMHO need to be) handled differently it would make sense for us to clarify this in the documentation of addContextListener (in both DesktopAgent and Channel ) and addIntentListener (in DesktopAgent) - the addition would need to be made both in TS src and docs pages. I think we should also add an error to ResolveError to return if a second intent listener is added for the same intent without first removing the old one.

I'd suggest that this is a clarification worth adding to the 2.3 scope. If you agree with the above and are happy to convert this question into an issue (replace 'Question' in the title with 'Clarify') I think we could agree a PR to resolve quite quickly.

Roaders commented 1 week ago

I think that it is quite likely that for a given app there might be multiple differnet parts in the app that might want to listen to a single intent being raised. One component might want to actually handle the intent and do something useful but another component might just want to display the context that is currently being handled.

I would say that this should probably be allowed - until @kriswest point about the getResult() implementation. It's going to be non-deterministic if you have multiple handlers all returning an async result. I guess whichever one returns first would be returned but this could lead to random results.

Because of this throwing an error when a second intent handler is registered for the given intent and app seems sensible to me.

kemerava commented 1 week ago

Thanks for the reply, @kriswest and @Roaders!

Originally I thought it would make sense to handle both of then with an error, especially since it seems so unintuitive to handle them differently (and, as you, @kriswest, pointed out, for the intent handler, that seems to be the only solution). However, through your examples it makes sense to me to have these behaviors be different and follow your approach.

Definitely agreed on a need for a very specific and clear documentation on this and a separate error. I will edit the title and, whenever I have bandwidth, will work on adding this change (unless someone does it before me)