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

align return types for callbacks, for declare_functions #70

Open
milyin opened this issue Dec 12, 2024 · 2 comments
Open

align return types for callbacks, for declare_functions #70

milyin opened this issue Dec 12, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@milyin
Copy link
Contributor

milyin commented Dec 12, 2024

Some functions returns Promise, some not. I don't see any system in this.
E.g.:

interface LivelinessSubscriberOptions {
    callback?: (sample: Sample) => Promise<void>;
}
export declare class Liveliness {
     declare_subscriber(key_expr: IntoKeyExpr, options?: LivelinessSubscriberOptions): Subscriber;

but

export interface QueryableOptions {
    callback?: (query: Query) => void;
}
export declare class Session {
    declare_subscriber(key_expr: IntoKeyExpr, handler?: ((sample: Sample) => Promise<void>) | Handler): Promise<Subscriber>;

Also why for liveliness' Subscriber callback is in options, but for session's one - in parameter and there is no options (dummy at this moment)

Need to go through ts API and align it. The zenoh-c and zenoh-cpp APIs can be used as a reference

@milyin milyin assigned milyin and Charles-Schleich and unassigned milyin Dec 12, 2024
@milyin milyin added the enhancement New feature or request label Dec 12, 2024
@Charles-Schleich
Copy link
Member

@milyin regarding the declare_subscriber in session, I was trying to replicate how zenoh Rust allowed either A handler or a callback.
In Zenoh Rust the user can call declare_subscriber
and either follow with a .callback() or a .with() which allows a handler.
I can put this in a options object if needed, but it is the only option that declare_subscriber takes, so i wasn't sure if it made sense to wrap it in an object seeing as it was the only one.

I also debated having something like an object containing

{
callback:...
handler:...
}

but the possibility of a user putting both in both a callback and a handler that they can poll seemed confusing from a usage perspective.

With regards to the Async and Promise, i agree it does need to be better aligned and cleaned up.
I've been using the Rust API as a reference, but a number of additional options in the builder pattern for the declare_x functions only exist on AdvancedPubSub which I believe was not planned originally for the Typescript API.

@milyin
Copy link
Contributor Author

milyin commented Dec 17, 2024

The zenoh-c API takes callbacks as a parameter of function. I think we should follow this approach. I.e. lets keep declare_subscriber as is, but e.g. move callback parameter to declare_queryable parameters from QueryableOptions like it's done in zenoh-c
Adding empty SubscriberOptions are not necessary for now. In zenoh-c it's empty dummy structure. But we need to be sure that it can be easiliy added in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

2 participants