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

Add 'once' to subscription options #270

Closed
wants to merge 2 commits into from
Closed

Conversation

zolkis
Copy link
Contributor

@zolkis zolkis commented Oct 7, 2020

No description provided.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok!
Just one question:

If |options|' |once| is true, invoke |subscription|'s
stop() method with |options|

This means that I can't use a different set of options for unsubscribing when using once parameter. Correct?

@zolkis
Copy link
Contributor Author

zolkis commented Oct 7, 2020

This means that I can't use a different set of options for unsubscribing when using once parameter. Correct?

The use case with once is that unsubscribe() is not called, therefore its |options| cannot be not specified. I think it is implementation specific how exactly stop() will use |options|. For use cases when unsubscribe options are needed, one could not use once, but implement it "manually" in the app. Actually this can be quite latent, which questions the actual utility of once.

@danielpeintner
Copy link
Contributor

danielpeintner commented Oct 8, 2020

I am not totally opposed to this extension but

  • it pushes complexity to the runtime (and if we open this door more is going to come)
  • it is a half-baked solution given that "what if I want to subscribe twice" or so?
    (maybe not a good example but I guess you know what I mean)

I tried to see what is necessary to do a one-time (or X-time) subscription (see variable subscriptionThreshhold)

// to keep track of subscription and how many times it has been called
let subscriptionThreshhold = 1;
let subscriptionCounter = 0;
let subscription: Subscription = null;
function handleSubscription(data: object) {
    if(subscription) {
        subscriptionCounter++;
        if(subscriptionCounter >= subscriptionThreshhold) {
            subscription.stop();
            // re-init if subscriptions can be done multiple times
            subscription = null;
            subscriptionCounter = 0;
        }
    }
}

// create thing and subscribe
let thing = new ConsumedThing();
subscription = thing.observeProperty("obs", (data) => {
    handleSubscription(data);
});

There might be better solutions. However, even this is not super complex and offers already the possibility to say subscribe 3 times by changing subscriptionThreshhold.

Note: the code snippet is not 100% correct since it does not use promises et cetera.

@zolkis
Copy link
Contributor Author

zolkis commented Oct 8, 2020

Right, so the balance for supporting once is like this:
Pro: convenience
Cons:

  • unsubscribe options missing
  • multiple subscriptions are complex to implement (this can be solved)
  • complexity with different protocol bindings.

Especially the first con looks bad, since introduces corner cases to the app. A convenience most of the time, when other subscribe options are not needed, but when they are needed, it cannot be used, so the app should be aware of what protocol binding is underneath, or otherwise there might be errors difficult to handle.

Occam says get rid of it from this low-level API and let the apps or convenience wrappers implement once.

@zolkis
Copy link
Contributor Author

zolkis commented Oct 8, 2020

Should I close without merging?

@danielpeintner
Copy link
Contributor

@relu91 what is your opinion?

@relu91
Copy link
Member

relu91 commented Oct 12, 2020

The reason I brought this topic up is that most of the known event APIs support the once option (i.e. Node EventEmitter and EventTarget). Therefore, I thought that developers might expect to have such functionality also inside a WoT runtime. However, I agree that missing unsubscribe parameters might be a problem.

So my final decision would be to drop this feature by now and reconsider later. Maybe when we have a fully stable subscription APIs.

@danielpeintner danielpeintner added enhancement Thoughts and ideas about possible improvements for next iteration Planned or postponed topics for the future labels Oct 12, 2020
@danielpeintner
Copy link
Contributor

danielpeintner commented Oct 12, 2020

Scripting API Call, 2020-1012:
Decision: No event-API so far... come back after possible subscription API changes

@zolkis
Copy link
Contributor Author

zolkis commented Oct 12, 2020

Closing for now.

@danielpeintner
Copy link
Contributor

FYI: just clicking on the enhancement label does not show this PR (given it is closed).

I created a dedicated issue to not lose track of it, see #274

@zolkis zolkis deleted the event-handling branch January 31, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Thoughts and ideas about possible improvements for next iteration Planned or postponed topics for the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants