Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Listener.unsubscribe() async for consistency #1300

Closed
1 of 6 tasks
Roaders opened this issue Jul 31, 2024 · 7 comments · Fixed by #1305
Closed
1 of 6 tasks

Make Listener.unsubscribe() async for consistency #1300

Roaders opened this issue Jul 31, 2024 · 7 comments · Fixed by #1305

Comments

@Roaders
Copy link
Contributor

Roaders commented Jul 31, 2024

Question Area

  • App Directory
  • API
  • Context Data
  • Intents
  • Use Cases
  • Other

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
Copy link
Contributor

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 Roaders changed the title Question: Listener.unsubscribe() Make Listener.unsubscribe() async for consistency Aug 1, 2024
@Roaders
Copy link
Contributor Author

Roaders commented Aug 1, 2024

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

@robmoffat
Copy link
Member

Related:

@Davidhanson90
Copy link

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
Copy link
Contributor Author

Roaders commented Aug 1, 2024

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
Copy link
Member

robmoffat commented Aug 1, 2024

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
Copy link
Contributor

kriswest commented Aug 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants