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

Yet another subscription API #268

Open
relu91 opened this issue Sep 30, 2020 · 9 comments
Open

Yet another subscription API #268

relu91 opened this issue Sep 30, 2020 · 9 comments
Labels
API-improvement Suggestions for changing the API enhancement Thoughts and ideas about possible improvements

Comments

@relu91
Copy link
Member

relu91 commented Sep 30, 2020

As requested here I wrote a new IDL for yet another variant of the subscription API. Please see the following Web IDL:

[Exposed=Window]
interface ConsumedThing {
  /*... other methods */
  ObservationHandle createObservation(DOMString name, optional InteractionOptions options = null);
  SubscriptionHandle createSubscription(DOMString name, optional InteractionOptions options = null);
};


[Exposed=Window]
interface AffordanceHandle : EventTarget {
     attribute EventHandler onerror;
};

[Exposed=Window]
interface SubscriptionHandle : AffordanceHandle {
     undefined subscribeEvent();
     undefined unsubscribeEvent();
     attribute EventHandler onunsubscribeevent;
     attribute EventHandler onevent;
     attribute EventHandler onsubscribeevent;
};

[Exposed=Window]
interface ObservationHandle : AffordanceHandle {
     undefined observeProperty();
     undefined unobserveProperty();
     attribute EventHandler onpropertychange;
     attribute EventHandler onobserveproperty;
     attribute EventHandler onunobserveproperty;
};

The design idea is to re-use as much as we can what is already out there. Therefore, here we are using EventTarget and EventHandler.

Pros

  • Compliant to [Web Platform Design Principles]https://w3ctag.github.io/design-principles/#dont-invent-event-like
  • no callback - promise duality
  • In the future, we could adapt this pattern also for actions. For example, if we find a way to define long-standing actions in the TD, we could define an ActionHandle
  • It still evokes TD basic operation verbs (i.e. observeProperty, onevent)

Cons

  • More verbose
  • Harder error handling for subscribe, unsubscribe, observe, and unobserve (i.e. you can't try {}catch{} but you have to use the EventHandler

What do you think? 🤔

@zolkis
Copy link
Contributor

zolkis commented Oct 1, 2020

First of all I would use constructors.
Then, for a single onerror the usual way is including it directly, instead of creating inheritance.
A variant of the above:

[Exposed=Window, SecureContext]
interface EventSubscription {
     constructor(DOMString name, optional InteractionOptions options = null);

     undefined start();  // could be left subscribe(), but this will look better in code examples
     undefined stop();
     attribute EventHandler onevent;
     attribute EventHandler onerror;

     // I don't think we need these, I would use Promise for start() and eventually stop()
     attribute EventHandler onunsubscribeevent;  
     attribute EventHandler onsubscribeevent;
};

[Exposed=Window, SecureContext]
interface PropertyObserve {
   // same stuff!
};

So in the end, we could merge the two kind of subscriptions to be handled by one interface,

[Exposed=Window, SecureContext]
interface Subscription {
     constructor(DOMString name, optional InteractionOptions options = null);

     Promise<undefined> start(); 
     undefined stop();

     attribute EventHandler ondata;
     attribute EventHandler onerror;
};

[Exposed=Window, SecureContext]
interface EventSubscription: Subscription;

[Exposed=Window, SecureContext]
interface PropertyObserve: Subscription;

There are reasons why stop() is not with a Promise, discussed extensively in other specs.
Code examples:

subscription = new EventSubscription("temperature");
subscription.ondata = interactionoutput => { ... };
subscription.onerror = error => { ... };
await subscription.start();
...
subscription.stop();
observe= new PropertyObserve("temperature");
observe.ondata = interactionoutput => { ... };
observe.onerror = error => { ... };
await observe.start();
...
observe.stop();

However, since the algorithms are different, we could specify the two kinds of subscriptions by duplicating the IDL (not a big deal).

Alternatively, we could go in the middle ground between this and your proposal, have start() and stop() return undefined and include the event trinity from Generic Sensors, onactivate, onreading, onerror.

@zolkis
Copy link
Contributor

zolkis commented Oct 1, 2020

But in the end I think the simplest would be to slightly modify the existing spec, re-introduce unsubscribeEvent() and unobserveProperty() (for the reason they have op words in the TD spec and we align to that), and eventually use subscription id's (TBD).

About the Promise-onerror duality. I think we need to distinguish between errors like SecurityError (coming from runtime policies) that appear during setting up the subscription process and the underlying protocol errors that might pop up during the operating subscription.

IMHO we just need to be clear in the algorithm how they are handled. The Promise fulfills when the subscription is successful. The error event (or callback) could be used for all the opaque protocol errors. I see this as clarity and not as clutter.

@relu91
Copy link
Member Author

relu91 commented Oct 1, 2020

First of all I would use constructors.
Then, for a single onerror the usual way is including it directly, instead of creating inheritance.

The reason because I introduced the AffordanceHandle was for extensibility. It helps if we'll ever have an ActionHanlde. The onerror event was the first thing that I found in common between the different interfaces, but I imagine that also other internal slots might be shared (i.e. Form, InteractionOptions, AffordanceName, etc.). Sorry I did not mention directly in the proposal.

Constructors might be fine, but we need to pass also a ConsumeThing instance, for obvious reasons:

interface EventSubscription {
     constructor(DOMString name, ConsumeThing thing,optional InteractionOptions options = null);
     // Other stuff
}

I used different classes to have different names for events and functions. I thought it was clearer and closer to the Thing Description interaction model. Although, one can say that it is better to use a single API like you are proposing.

There are reasons why stop() is not with a Promise, discussed extensively in other specs.

So in your API we are missing a way to know when you are successfully unsubscribed/unobserved. I guess we should introduce an event.

So in the end it would be something like this:

[Exposed=Window, SecureContext]
interface Subscription {
     constructor(DOMString name, ConsumeThing thing,optional InteractionOptions options = null);

     Promise<undefined> start(); //no direct mapping with what is happening at the platform level (i.e. form with op subscribe/observer)
     undefined stop();//no direct mapping with what is happening at the platform level (i.e. form with op unsubscribe/unobserver)

     attribute EventHandler ondata;
     attribute EventHandler onerror;
     attribute EventHandler onstop; 
};

[Exposed=Window, SecureContext]
interface EventSubscription: Subscription;

[Exposed=Window, SecureContext]
interface PropertyObserve: Subscription;

Even if I still not convinced that start should be a Promise. We are again mixing two different programming styles 🤔 .

@relu91
Copy link
Member Author

relu91 commented Oct 1, 2020

But in the end I think the simplest would be to slightly modify the existing spec, re-introduce unsubscribeEvent() and unobserveProperty() (for the reason they have op words in the TD spec and we align to that), and eventually use subscription id's (TBD).
About the Promise-onerror duality. I think we need to distinguish between errors like SecurityError (coming from runtime policies) that appear during setting up the subscription process and the underlying protocol errors that might pop up during the operating subscription.
IMHO we just need to be clear in the algorithm how they are handled. The Promise fulfills when the subscription is successful. The error event (or callback) could be used for all the opaque protocol errors. I see this as clarity and not as clutter.

Subscription ids feel a lot like File Descriptors... can't we use the functions passed as callbacks as identifiers? 🤔

@zolkis
Copy link
Contributor

zolkis commented Oct 1, 2020

So in your API we are missing a way to know when you are successfully unsubscribed/unobserved.

We could have a Promise on stop(), that is cheap, but it's only a syntactic thing that looks like a solution. Usually cancellation cannot be guaranteed in all cases. Suppression is a workaround, of course, but if we are exact, the function cannot guarantee the result reported, max simulate/fake it in the worst cases.

We are again mixing two different programming styles

Yes, because each is used for its designated purpose.
The Promise is meant for long-ish operations that can either succeed or fail. A network request, for instance. Like subscribe request.
Then, the error event is meant for notifying about operational errors during a longer operation, like the lifetime of a subscription.
IMHO the mix-up would be if we didn't separate the errors for the subscribe request, and for the errors that might raise during the subscription lifetime. Anyway, a single error event is doable.

However, we have a long history/debate about avoiding events in the API design, because the EventTarget - Node EventListener controversy, and it goes back for many years. For this iteration, I would stay with callbacks: clean and clear, no uncertainty what actually is implemented from EventTarget in a Node or in a resource-restrained WoT runtime environment. Callbacks are far more efficient, if they don't blow up the API (too much).

About subscription id's: yes, we can use the functions as well, but IIRC @danielpeintner had an argument against that and preferred id's.

@zolkis
Copy link
Contributor

zolkis commented Oct 1, 2020

//no direct mapping with what is happening at the platform level (i.e. form with op subscribe/observer)

I don't understand that comment, since the current algorithms will all remain, i.e. it is very much connected to the forms.
Stopping already has a clause for suppressing after unsubscribe/unobserve.

@relu91
Copy link
Member Author

relu91 commented Oct 2, 2020

However, we have a long history/debate about avoiding events in the API design, because the EventTarget - Node EventListener controversy, and it goes back for many years. For this iteration, I would stay with callbacks: clean and clear, no uncertainty what actually is implemented from EventTarget in a Node or in a resource-restrained WoT runtime environment. Callbacks are far more efficient, if they don't blow up the API (too much).

FYI Node 14 has native experimental support for EventTarget and Events: https://nodejs.org/api/events.html#events_eventtarget_and_event_api.

Of course, there is also a polyfill library, although not so popular. On the other hand, I have to admit that callbacks might be easier (more efficient maybe?) to implement on constraint devices runtimes. We don't have to forget that we are designing APIs also for these devices, you're right.

I don't understand that comment, since the current algorithms will all remain, i.e. it is very much connected to the forms.
Stopping already has a clause for suppressing after unsubscribe/unobserve.

I meant that a start() method does not immediately evoke the fact that a subscribeevent op will be called, whereas using subscribe() is more direct.

@zolkis
Copy link
Contributor

zolkis commented Oct 2, 2020

I meant that a start() method does not immediately evoke the fact that a subscribeevent op will be called, whereas using subscribe() is more direct.

Ah, that is true, yet it's quite direct 1:1 mapping, so it should not be a problem.

However, I would prefer if we found a solution using ConsumedThing::subscribe().

@danielpeintner danielpeintner added the enhancement Thoughts and ideas about possible improvements label Oct 12, 2020
@relu91
Copy link
Member Author

relu91 commented Dec 11, 2023

I think we should re-evaluate the design proposal with the current approach. I think several proposes have been blended together. If we don't find any issue with the current design I think we can close this.

@JKRhb JKRhb added the API-improvement Suggestions for changing the API label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-improvement Suggestions for changing the API enhancement Thoughts and ideas about possible improvements
Projects
None yet
Development

No branches or pull requests

4 participants