Smart Coffee Machine - Fixes/Updates/Proposal

[danielpeintner]

I would like to split the issue in 4 sections about the smart-coffee-machine samples in https://github.com/eclipse/thingweb.node-wot/tree/master/packages/examples/src/scripts

1. Bugs

The code uses promises in the wrong way (see here or here, ..).

There should be one promise and errors should return reject and not resolve.

    thing.setPropertyWriteHandler('availableResourceLevel', (val, options) => {
        return new Promise((resolve, reject) => {
			// Check if uriVariables are provided
			if (options && typeof options === 'object' && 'uriVariables' in options) {
				const uriVariables: any = options['uriVariables'];
				if ('id' in uriVariables) {
					return thing.readProperty('allAvailableResources').then((resources) => {
						const id = uriVariables['id'];
						resources[id] = val;
						return thing.writeProperty('allAvailableResources', resources);
					});
				} else {
					reject('...');
				}
			} else {
				reject('Please specify id variable as uriVariables.');
			}            
        });
    });

Note: I did not check the code for validity but I think you get the problem (difference to your code).

2. Best Practices

Should we actually propose using uriVariables. I think this is something WoT needs to support given it exists but I suggest using variables in the body.

3. Improvements

It would be nice IF the makeDrink action is a longer running process. Hence a coffee should not be ready immediately. Moreover... in reality makeDrink quickly one after the other should fail given that the coffee machine is not ready yet.

Hence I also propose to remove the maintenanceNeeded property and use a more generic status that could be an enumeration like ["maintenanceNeeded", "inUse/brewing..", ...]

Doing so would also allow modelling the brewing process that takes some time and would refuse new drinks till the old on is ready or the machine is maintained...

4. WebUI improvements

With the addition of a status property we can also improve the UI hosted here: http://plugfest.thingweb.io/examples/smart-coffee-machine.html

The machine could be blinking while brewing the coffee.

Note: It is also necessary that you put the link where you get the SVG file for the coffee machine.

@fatadel can you take a look (I think 1. should be fixed. I don't feel strongly about the rest)
@sebastiankb @egekorkan @relu91 others, please comment if you have another opinion

[danielpeintner]

relates to eclipse-thingweb/website#19

[egekorkan]

Some other things I have seen:

• producing and exposing are not the same thing. The text says "Creating a WoT Thing is called exposing or producing a Thing".
• " Note that, all affordances (i.e. property, action and event) should be added withing the produce method." withing -> within
• servedCounter write handler has resolve before the logic. Also explaining what resolve does here is helpful :)
• "Now we need to set up action handlers, which proceed when another Thing or client invokes the action." -> swapping client with Consumer
• "Notice, how in case of insufficient resources the outOfResource eve" -> removing comma
• "Now our final affordances, that is Event Affordances. " -> swap is with are
• In node-wot this process is called “subscribing” -> in WoT instead of node-wot
• In event affordances, I would repeat the code above that does event emission
• It would be better to explain what happens when the expose method finishes (resolves).
• "and coap://127.0.0.1:5683/smart-coffee-machine for CoAP" -> and at coap:// ....
• "By default, in HTTP binding the GET met..." -> "in HTTP binding**,** the"
• "A property can be written using thing.writeProperty me" -> "written to"
• "Notice on usage of uriVariables here. I" -> swap on with the
• "Notice that, here we don’t need to await for a function to complete, since observing a property is a persistent action." This is not clear. Should be reworded to say that it continuosly delivers data and that data should be handled each time in a callback or something similar. Similar case in the event example
• "This can be well noted on invoking of setSchedule action." -> A bit broken sentence
• "Currently, node-wot supports different security schemas." -> schemes instead of schemas

[egekorkan]

Also, links to examples return 404: E.g. at "The full script is available at node-wot GitHub repository. "

[danielpeintner]

Also, links to examples return 404: E.g. at "The full script is available at node-wot GitHub repository. "

This is my fault given I renamed the examples consistently to "smart-coffee-machine". Hence we need to fix the links as well...

[fatadel]

Also, links to examples return 404: E.g. at "The full script is available at node-wot GitHub repository. "

This is my fault given I renamed the examples consistently to "smart-coffee-machine". Hence we need to fix the links as well...

No worries, I've made a PR for that as I said - eclipse-thingweb/website#23.

[fatadel]

@danielpeintner @egekorkan
The reason why promises are made like that is the following.
The first picture is the current implementation. The second one is using reject the way you wish.

In the first case, a user sees what is wrong. In the second, a user just infinitely awaits a response and has no clue what's wrong.
See also my comment - eclipse-thingweb/website#19 (comment).

But if you insist I will rewrite it now with the reject methods.

[danielpeintner]

The problem is that if you resolve with a text explaining the reason there is no way to detect that something went wrong. Errors should reject with the appropriate error message.

Note: if your REST tools works correctly and the code is working correctly (there are issues also as I mentioned) the tools should show you the error just fine.

This is not because I am insisting to do so ;-)

[egekorkan]

I also don't understand how reject is not delivered but it can be also because you are not wrapping the logic inside a promise

[fatadel]

The problem is that if you resolve with a text explaining the reason there is no way to detect that something went wrong. Errors should reject with the appropriate error message.

Note: if your REST tools works correctly and the code is working correctly (there are issues also as I mentioned) the tools should show you the error just fine.

This is not because I am insisting to do so ;-)

I get the same behavior in the Chrome browser and Insomnia REST client, so no doubt they work correctly. Apparently, there is a bug somewhere else or this code is not 100% correct.

I also don't understand how reject is not delivered but it can be also because you are not wrapping the logic inside a promise

No, I do wrap it in the second case.

[egekorkan]

Ah I see, my bad. This must be some error in node-wot then :/

[danielpeintner]

I am still not 100% convinced that it is actually an issue in node-wot since I recall getting rejects. @fatadel is this behavior still the case for your recent updates in https://github.com/eclipse/thingweb.node-wot/pull/337/files ?

If so, please provide a simple a simple test cases for https://github.com/eclipse/thingweb.node-wot/blob/master/packages/core/test/ServerTest.ts that shows the issue

[fatadel]

I am still not 100% convinced that it is actually an issue in node-wot since I recall getting rejects. @fatadel is this behavior still the case for your recent updates in https://github.com/eclipse/thingweb.node-wot/pull/337/files ?

If so, please provide a simple a simple test cases for https://github.com/eclipse/thingweb.node-wot/blob/master/packages/core/test/ServerTest.ts that shows the issue

Done!

[relu91]

FYI: using async and await makes the code much more cleaner:

thing.setPropertyWriteHandler('availableResourceLevel',  async (val, options) => {
      // Check if uriVariables are provided
      if (options && typeof options === 'object' && 'uriVariables' in options) {
           const uriVariables: any = options['uriVariables'];
	   if ('id' in uriVariables) {
		const resources = await thing.readProperty('allAvailableResources');
                const id = uriVariables['id'];
		resources[id] = val;
		return thing.writeProperty('allAvailableResources', resources);
	  } else {
		throw new Error('...');
	  }
     else {
         throw new Error('Please specify id variable as uriVariables.');
     }            
});