finos / FDC3

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

Make Listener.unsubscribe() async for consistency #1300

Closed Roaders closed 2 months ago

Roaders commented 3 months ago

Question Area

Question

Nearly all of the functions in the api are async and return a Promise. Listener.unsubscribe() is not.

I can see that it could well be a fire and forget function but within BrowserTypes we have ContextListenerUnsubscribeRequest, ContextListenerUnsubscribeResponse, IntentListenerUnsubscribeRequest and IntentListenerUnsubscribeResponse.

This leads me to assume that when we call unsubscribe we need to dispatch a Request message and wait for a Response message. In the meantime we probably aren't unsubscribed.

Is this assumption incorrect? Should we unsubscribe immediately before we get the response message? In which case what is the point of the response message? How do we handle errors with the unsubscribe process?

kriswest commented 3 months ago

Listener.unsubscribe() probably should have been updated to be async (i.e. chnage return type from void to Promise<void>), which would be a non-breaking change that I think would be worth making for consistency.

I would think most implementations handle this in a fire and forget form currently, and I doubt we'd have objections to making it async. If you agree please update the issue title (say to "Make Listener.unsubscribe() async for consistency") and I'll happily take on the tasks of a PR to change it.

Roaders commented 3 months ago

I assume the return type would be Promise<void> | void so as not to break current implementations?

robmoffat commented 3 months ago

Related:

Davidhanson90 commented 3 months ago

I assume the return type would be Promise<void> | void so as not to break current implementations?

The semantics of awaiting this could be pretty poor as would require null check before await. Also where do you envisage a break?

listener.unsubscribe() //Void

listener.unsubscribe() //Promise

will still result in the same behavior as far as I can see. Though obviously there is no guarantee in either unless you were to await.

Roaders commented 3 months ago

After having an argument conversation with @Davidhanson90 about this I should clarify... I was talking about breaking implementations not consumers of the api. I guess though that this would be introduced in 2.2 so for an agent vendor to be 2.2 compliant they would have to update to this.

Non breaking from consumer point of view (which is what the versioning is for), breaking from implementors point of view.

robmoffat commented 3 months ago

I assume the return type would be Promise | void so as not to break current implementations?

Is going from void -> Promise a breaking change? I wouldn't expect it to be from the point of view of apps.

kriswest commented 3 months ago

Yes thats the conclusion we came to before, for example when fdc3.broadcast had the same change applied. If the return type used to void then adding a return type breaks little, but does require a small update from a vendor for the additive change.