Update template #619
Update template #619
Conversation
| if (tdDirectory) { | ||
| this.register(tdDirectory); | ||
| } | ||
| this.listen_to_myEvent(); //used to listen to specific events provided by a library. If you don't have events, simply remove it | ||
| this.listenToMyEvent(); // used to listen to specific events provided by a library. If you don't have events, simply remove it |
There was a problem hiding this comment.
Since this is asynchronous it would be better to have a start method where the thing got produced and exposed. I would refactor it since we are modifying the template!
| resolve(); | ||
| // write something to property | ||
| this.myProperty = inputData; | ||
| // resolve that write was succesful |
There was a problem hiding this comment.
it would be good to show emitting observed properties
There was a problem hiding this comment.
I think we are missing: https://w3c.github.io/wot-scripting-api/#the-emitpropertychange-method . Is going to be added after I've done reviewing: relu91#17
| this.add_actions(); | ||
| this.add_events(); | ||
| this.thing.expose(); | ||
| this.addProperties(); // Initialize properties and add their handlers |
There was a problem hiding this comment.
I would also propose renaming these methods. They were named like this from the pre 0.7.x times where each affordance was added via methods and not via the produce(). A possible name could be addPropertyHandlers but there is also initialization happening.
| this.thing.emitEvent("myEvent") // Emiting an event (may be removed; only for demonstration purposes) | ||
| } | ||
| //resolve that action was successful | ||
| resolve(true); |
There was a problem hiding this comment.
doesn't resolve here correspond to the outputData ?
| } | ||
|
|
||
| private add_actions() { | ||
| private addActions() { |
There was a problem hiding this comment.
contrary to my comment about properties, this should be called addActionHandlers
| // write something to property | ||
| this.myProperty = inputData; | ||
| // resolve that write was succesful | ||
| resolve(true); |
There was a problem hiding this comment.
not sure what the true value does here, as in I am not sure of the behavior
|
Note: @FadySalama you also need to sign Eclipse ECA |
There was a problem hiding this comment.
Another, neat pick can we use unknown instead of any as the return type of action and property handler? Using any is usually considered a bad practise and I would discourage using it in a template. (To be honest users will need to replace the type anyway, but I think it is good to provide an already well styled code)
|
@FadySalama do you plan to update this PR? |
|
@egekorkan please let us know whether your comments were also revolved. |
|
If I am not wrong, none of my comments are fixed @FadySalama ? |
I am pretty sure I did @egekorkan. Can you read the file again and point out which points weren't changed? |
|
My bad, good to be merged :) |
This pull request updates the template to version v.0.8.x