eclipse-thingweb / website

Homepage for Eclipse Thingweb, thingweb.io
https://thingweb.io
Other
5 stars 11 forks source link

Coffee Machine Tutorial has misleading parts #15

Closed egekorkan closed 4 years ago

egekorkan commented 4 years ago

@fatadel Your coffee machine tutorial has some misleading parts.

  1. In the event affordances, you say:
    
    // Set up a handler for outOfResource event     

thing.subscribeEvent('outOfResource', (data) => {
// Notify an "admin" when the event is emitted
// (the notify function here simply logs a message to the console) notify('admin@coffeeMachine.com', outOfResource event: ${data});
});


This is not really a handler for the events since events do not have a handler like properties and actions do, you just emit them. By the way you emit them in the part of Action Affordances anyways, so you can call the notify function in there and not create another listener for this. I am guessing that you did this to be _symmetrical_ to other affordances but I think that we should not encourage this. The text gives the impression that there is some real event handling happening.

2. You have a Property Affordance called `allAvailableResources`. However, node-wot automatically generates handlers for meta interactions like `readallproperties`. Were you aware of this or is this property there for another reason?

3.Also, there is no name displayed on the page/tab. Similarly, the hands-on page displays `thingweb-thingweb`. Could you take a look at it @danielpeintner ?
danielpeintner commented 4 years ago

3.Also, there is no name displayed on the page/tab. Similarly, the hands-on page displays thingweb-thingweb. Could you take a look at it @danielpeintner ?

https://github.com/eclipse/thingweb.website/commit/33eb9f2c4f5d72c02c5f78e7b4165f76993d2273 fixes the issue

fatadel commented 4 years ago

@egekorkan Thank you very much for your feedback!

  1. I couldn't really understand this point. If there is an event you usually also want to subscribe to that particular event, don't you? Otherwise, nothing will happen when the event is emitted, which seems quite silly for me. So, the code shows how to do it. I know that I could simply invoke the notify function when there are no sufficient resources, but the idea was to show how events can be used in that case. Additionally, instead of just notifying, the event handler (or listener, whatever you call it) can be much more sophisticated and it's just simplified for the sake of the example.

  2. I am aware of the readallproperties method but this was done intentionally. The idea was to show the usage of uriVariables with Property Affordances, and, as we have discussed in Telegram, this usually involves having another property for the same data since you cannot read/write to the same property within your own read/write handler.

egekorkan commented 4 years ago

About the second point, all good!

About the first one:

You subscribe to an event as the Consumer and not really as the Exposed Thing itself. Once you call the emit function, the event is sent to its subscribers on all protocols. What is very important to show is that there is no Event Handler in the Exposed Thing, you do not handle events, you emit them.

fatadel commented 4 years ago

I have handlers in both client and server Thing. So, you suggest removing the server part and leaving it only inside the client?

egekorkan commented 4 years ago

That is what I would suggest to not give the wrong impression. You can however say that such a behavior is possible.

fatadel commented 4 years ago

I will resolve this as soon as the PRs for the coffee machine + oAuth will be merged in order to avoid merge conflicts. I hope it is not asap, @egekorkan?

egekorkan commented 4 years ago

Yeah it's not super urgent but should not how we teach node-wot for sure

fatadel commented 4 years ago

@egekorkan Please find out these two PRs closing this issue: https://github.com/eclipse/thingweb.node-wot/pull/248 https://github.com/eclipse/thingweb.website/pull/20

egekorkan commented 4 years ago

Issue can be closed after the merge of the PR linked above