Update the API according to discussion in #82 and #78 #86

zolkis · 2018-01-29T11:57:11Z

Don't merge yet, this is work in progress.
For the rendered version of the tip of the PR, use this link.

Signed-off-by: Zoltan Kis [email protected]

Signed-off-by: Zoltan Kis <[email protected]>

…posedThings. See also w3c#64. Signed-off-by: Zoltan Kis <[email protected]>

…eDefinition, request handlers for ExposedThing Signed-off-by: Zoltan Kis <[email protected]>

mkovatsc · 2018-01-31T15:31:01Z

This PR introduces many completely new concepts that conflict with our long-lived discussion in #82, e.g.:

expose vs produce
typedef ThingModel and reinterpretation of ThingTemplate
ExposedThing without ConsumedThing interface (no control over the Thing)
Cut down Interaction Inits
dictionary ValueDefinition
Additional set*Handler() functions

Is this to be changed as part of the work-in-progress, or are you proposing this now?

Signed-off-by: Zoltan Kis <[email protected]>

zolkis · 2018-01-31T15:52:04Z

We have addressed these points during the call. Now the proposal is more or less clean. The reasons for change are the following.

expose vs produce
This is mainly a name change, coming from the fact that the returned object (ExposedThing) is not yet an exposed Thing, but an object that needs to or might undergo changes such as adding properties/actions/events and request handlers. Please read the text, the current proposal allows creating an ExposedThing in 3 ways:

based on a model (name + semantic annotations) that needs to be modified by adding properties/actions/events/handlers
based on a TD, that needs to be added handlers
based on a ConsumedThing (actually its TD, but implementations may do optimizations), that needs to be added handlers.

Also, see #64 for reasoning on consume/produce pair of words. While I think "expose()" made sense when the returned object was ready to be started, this method is really just a factory for a half-baked exposed Thing. Perhaps we could rename ExposedThing, but we don't need to. This method could also be just a constructor for an ExposedThing instead of a factory method. That would be more clear, and also would permit better separation/grouping in the spec.

ThingModel reflects the possibilities from above, including ThingTemplate (that remained the same as in API for next PlugFest (Prague 2018) #82).
Actually a script has total control over an ExposedThing. It defines the thing, so cannot use it before it is defined. If ConsumedThing interface is needed, it needs to consume the ExposedThing's generated TD.

Cut down Interaction Inits

I am not sure what do you mean here. Perhaps 6. explains.

ValueDefinition is an attempt to align with the TD spec. Before this a full ThingDescription was used for specifying types - which was wrong and confusing.
The set*Handler() functions are needed for two reasons:

it is an antipattern to specify function in init dictionary members
they make explicit the decision a TD does not contain scripts. Before, if you wanted to create and ExposedThing based on a template/TD, and wanted to add code (that was not present in the template/TD) you first needed to remove e.g. a property/action, then re-add it with the handler mentioned in the init dict. This was just wrong. Now you can take a template/TD/ConsumedThing, replicate the TD-related structures and add the handlers on top. Also, it is clear at the top level that an ExposedThing must specify handlers, it is not any more hidden behind the members of a parameter of a function.

In fact, part of these changes are small, part of them are needed. The question is consistency, usability and alignment with the TD spec and other APIs. I believe this version is better (if not perfect) in that sense.

mkovatsc · 2018-01-31T16:59:31Z

Mainly, we must not repeat the mishap of a broken (FP)WD, which was caused by ad hoc changes and introduction of unfinished, unconsolidated concepts. So why introduce these changes now and independently? The conclusion in the call was to write up what was discussed there.

expose vs produce -> fine by me
Okay, small rename to be more precise, but also potentially more confusing in regard to established terminology.
ThingModel -> partly fine
Ideally, ThingTemplate and ThingDescription would become the same; also only a subset of a TD is valid for exposing Things (e.g., not the binding information). Thus, ThingTemplate was introduced as intermediary solution. For now, we could allow for an object (ThingTemplate) or a string (ThingDescription), which is similar to an earlier version of the API.
To put a ConsumedThing there seems to only fulfill a specific use case of optimizing proxy Thing creation and opens the path for the strange pattern of 3. This should not be introduced, definitely not for Prague because of the implications on the necessary implementation.
ExposedThing control -> not fine by me
It is pretty ugly to juggle two objects, an ExposedThing and a ConsumedThing, to realize a fundamental use case: implementing the internals of a Thing. The writeProperty() function of an ExposedThing is different from the one of ConsumedThing: an ExposedThing is under the local control of the code and directly manipulates the state, while a ConsumedThing only sends request to modify a remote Thing under control of someone else. It was convenient to let ExposedThing implement the interface of ConsumedThing, but this example here tells me, it would be clearer to fully specify both interfaces independently. This was also raised in one of the calls with a similar motiviation.
Interaction Inits -> might be fine
Weird that this comes up only now, as we are basically going back to the initial API (cf. onInvokeAction() etc.). This in itself might be a good sign, though.

mkovatsc · 2018-01-31T17:00:04Z

@knimura and @danielpeintner please comment, so we can have a prepared call on Monday.

zolkis · 2018-01-31T20:02:29Z

expose vs produce -> fine by me

The remaining question is whether should we rather make it a constructor to ExposedThing instead of a factory method with disputed naming? Then we avoid the terminology discussion and have cleaner, more JS-like definition to create local ExposedThing objects. The only reason to keep the factory method would be if we wanted to create remote Things, but that is no longer a use case, as it can be done as a service.

The WebIDL would look like this:

[Constructor(optional ThingModel model)]
interface ExposedThing {
   //...
};

ThingModel -> partly fine

I am also fine with removing ConsumedThing from the models. I was just thinking it could offer significant optimization opportunity when we want to re-expose a consumed thing, i.e. we don't need to fetch/consume a TD, then read the serialized form from the consumed thing and feed it to the ExposedThing factory that will parse it again. However, we could add this later, eventually.

ExposedThing control -> not fine by me

I am also fine with ExposedThing extending ConsumedThing, but we should be clear about the use cases.
The ExposedThing will define the properties, actions and events, then how to update/read a property, how to call an action, when to emit an event etc. So why would it want to use ConsumedThing methods to call an action, update/read a property and attach event listeners? It is implementing and exposing them, not using them as a client, not until finished exposing. There are two stages here, 1. exposing, 2. using (after consuming). I thought to make that distinction more clear. Perhaps we need to think what is the best way to expose the ConsumedThing interface on an ExposedThing, and when.

Interaction Inits -> might be fine

After playing with examples, I am pretty convinced now that this is the best way to go. I should add more example code to the spec.

mkovatsc · 2018-01-31T22:36:33Z

Postpone discussing the constructor idea after Prague or prepare options for a F2F breakout.
Good, then let's limit to ThingTemplate dictionary or TD string. ConsumedThing.description can be passed and does not need a fetch.
It's not about extending ConsumedThing, but implementing the interface, as we need similar functions to control the local state. Consuming usually happens in a remote runtume. The implementation of the functions will be fundamentally different, yet I don't think we will need different names. At least postpone this discussion.
Let's bounce this of the other editors before the call.

…dThing. Signed-off-by: Zoltan Kis <[email protected]>

knimura · 2018-02-02T08:31:20Z

expose vs produce

No issues with the name change.

ThingModel – what is ThingModel

Not clear the definition of ThingModel. I have no problem with ThingTemplate, if the definition of ThingTemplate is the complete subset of TD but just excluding binding information from it.

“Actually a script has total control over an ExposedThing. It defines the thing, so cannot use it before it is defined. If ConsumedThing interface is needed, it needs to consume the ExposedThing's generated TD.”

I agree with ExposedThing extending ConsumedThing, because ExposedThing should have the same function as ConsumedThing. But maybe difficult to understand the notion of "inheriting client API of local thing is part of ExposedThing(ServerAPI)", so showing the use case would be needed.

ValueDefinition is an attempt to align with the TD spec. Before this a full ThingDescription was used for specifying types - which was wrong and confusing.”

Totally agree.

“The set*Handler() functions are needed for two reasons:
If you wanted to create and ExposedThing based on a template/TD, and wanted to add code you first needed to remove e.g. a property/action, then re-add it with the handler mentioned in the init dict.

Agree. If ThingTemplate is subset of TD and ExposedThing is created from ThingTemplate, both removeProperty and addProperty must be called to setting handler. If we feel that going back to the initial API, introducing UpdateProperty may be one of the solution to avoid onRetrieveProperty or so.

…dlers. Signed-off-by: Zoltan Kis <[email protected]>

zolkis · 2018-02-02T14:07:25Z

Not clear the definition of ThingModel.

A Thing model (the term was taken from #64) is either a ThingTemplate (dictionary) or a ThingDescription (string).

I have changed back to ExposedThing implements ConsumedThing.

Signed-off-by: Zoltan Kis <[email protected]>

…nd dictionary members. Signed-off-by: Zoltan Kis <[email protected]>

zolkis · 2018-02-05T15:12:49Z

As discussed on the call, on request by @knimura , added a short informal section for Observables, and on request by @danielpeintner , removed optionality from some of the arguments and dictionary members.

zolkis · 2018-02-05T15:42:36Z

As discussed in the call, added explanations on how handlers are working/implemented on ExposedThing. @mjkoster please take a look.

Signed-off-by: Zoltan Kis <[email protected]>

danielpeintner · 2018-02-07T14:12:58Z

One comment/question: I noticed that PropertyWriteHandler gets only the new value. We used to have "oldValue" and "newValue". Any rational for this change?

zolkis · 2018-02-07T14:21:17Z

WiP is removed, proof-reading can start.

mkovatsc · 2018-02-07T14:22:23Z

One comment/question: I noticed that PropertyWriteHandler gets only the new value. We used to have "oldValue" and "newValue". Any rational for this change?

This is a quite strong concept change. As I mentioned, we had a reactive programming pattern before, which has at least two benefits:

Script developers do not need to care about proper and safe state management. Without setting a PropertyHandlers, we have a basic resource behavior (set state with PUT, read state with GET).
Better performance, as requests can be answered directly from the runtime space, no need to hand over to the scripting space (which was a bit problematic with our previous Java + Nashorn implementation and will be problematic for embedded runtimes).

Such a change must be explicitly discussed and is not fit for Prague.

zolkis · 2018-02-07T14:29:59Z

There is an editor's Note in the spec stating:

Note that this function is invoked by implementations before the property is updated, so the client code in this function can invoke the readProperty() method to find out the old value of the property, if needed. Therefore the old value is not provided to this method.

So your use case seems to be solved.
Providing an old value would create confusion because what does it mean: which old value? It might be different than the value read by readProperty() in a given client.

mkovatsc · 2018-02-07T14:52:42Z

We are discussing server side (ExposedThing) -- the client (ConsumedThing) has nothing to do with these handlers.

mkovatsc · 2018-02-07T15:18:11Z

Okay, I see you already removed "client" from that note -- thanks! ;)

Have you checked that the handler code has always the right reference to the ExposedThing object to call readProperty(propertyName)? To me it is not obvious without testing, also function vs lambda...

zolkis · 2018-02-07T15:26:01Z

Of course the handler code has the right reference to the ExposedThing object.

thing.setPropertyWriteHandler(value => {
  thing.readProperty(propertyName).then(oldValue => {
    // put handler code here
   if (oldValue < 5) {
      // update to value 
      gpio.port.write(0x02, value);
   } 
  });
}, propertyName);

…ons. Removed duplicated statements. Signed-off-by: Zoltan Kis <[email protected]>

danielpeintner

Please find comments I got when doing a first read through. In general I think it is fine to merge and keep fixing/updating.

danielpeintner · 2018-02-12T09:22:39Z

index.html

+        let discoveryFilter = {
+          method: "directory",
+          url: "http://directory.wotservice.org"
+        };
        let subscription = wot.discover(discoveryFilter).subscribe(
          thing => { console.log("Found Thing " + thing.url); },


A ConsumedThing does not have url field anymore. Example needs to be change to thing.name

danielpeintner · 2018-02-12T09:23:29Z

index.html

+            from a <a>Thing Description</a>, then adding request handlers;
+          </li>
+          <li>
+            from an existing <a>ConsumedThing</a> object, based on its <a>Thing Description</a>, then adding request handlers.


The third option is a special case of option 2. Hence I propose to remove option 3 and change option 2 to

from a Thing Description (possibly also retrieved from a ConsumedThing), then adding request handlers;

danielpeintner · 2018-02-12T09:23:49Z

index.html

            USVString url;
-            Dictionary description;
+            USVString query;
+            sequence&lt;Dictionary&gt; constraints;


The AND and OR relationship for constraints seems to be very powerful but also complex at first. Moreover, the link to "the semantic metadata" in its description is very fuzzy. As I understand it, the key of the key-value pair is a reference to SemanticType. If so we might want to add an example.

See Example 4. ("protocol" is made up for the example)

danielpeintner · 2018-02-12T09:25:41Z

index.html

-        let subscription = wot.discover({ method: "nearby", description: {protocol: "BLE4.2"} }).subscribe(
+      <pre class="example" title="Same as above but with different Observable syntax">
+        let subscription = wot.discover({ method: "local" }).subscribe({
+          next: thing => { console.log("Found local Thing " + thing.url); },


See before: A ConsumedThing does not have url field anymore. Example needs to be change to thing.name

danielpeintner · 2018-02-12T09:25:52Z

index.html

+        let subscription = wot.discover({
+          method: "nearby",
+          constraints: [{ protocol: "BLE-4.2" }, { protocol: "NFC"}]
+        }).subscribe(
          thing => { console.log("Found nearby Thing " + thing.url); },


See before: A ConsumedThing does not have url field anymore. Example needs to be change to thing.name

danielpeintner · 2018-02-12T09:56:58Z

index.html

+          dictionary ThingPropertyInit: SemanticAnnotations {
+            required DOMString name;
+            required ValueType type;
+            any value;
            boolean configurable = false;


The latest TD draft (https://w3c.github.io/wot-thing-description/#property) supports observable and writable.

We need to check what will/should be the default value.

danielpeintner · 2018-02-12T09:58:13Z

index.html

-            DOMString name;
+          dictionary ThingPropertyInit: SemanticAnnotations {
+            required DOMString name;
+            required ValueType type;


Align with TD draft (call it outputData) ?

danielpeintner · 2018-02-12T10:01:01Z

index.html

-            Function action;
+          dictionary ThingActionInit: SemanticAnnotations {
+            required DOMString name;
+            ValueType inputDataDescription;


Align with TD draft, name it inputData and outputData?

inputData would imply this is data, but it is a description of input data.

danielpeintner · 2018-02-12T10:01:42Z

index.html

-            ThingDescription dataDescription;
+          dictionary ThingEventInit: SemanticAnnotations {
+            required DOMString name;
+            ValueType dataDescription;


Align with TD draft, name it outputData?

outputData is a bad name, usually events have "data" in JS. This API is for developers.

danielpeintner · 2018-02-12T10:07:43Z

index.html

@@ -962,31 +837,46 @@ <h2>Examples</h2>
      <pre class="example highlight" title="Create a new blank exposed Thing">
        WoT.createExposedThing(thingDescription)


Old API example. Should be WoT.produce without Promise and then etc

Also in addAction there is still old handler code.

Thanks for the catch. Done.

Signed-off-by: Zoltan Kis <[email protected]>

… discovery constraint matching. Signed-off-by: Zoltan Kis <[email protected]>

zolkis · 2018-02-13T12:42:46Z

There is still a lot of work needed on SemanticAnnotations and DataSchema, but for the moment this is good to be merged.

zolkis · 2018-02-13T12:43:43Z

I get a ReSpec warning for undefined Observable, but it's defined. Someone please take another look and try to fix.

danielpeintner · 2018-02-13T13:11:20Z

Merging is fine by me also!

Note: the ReSpec issue you are seeing relates to the observable attribute in "5.7.1 The ThingPropertyInit dictionary". The name collision seems to cause that issue. Renaming the attribute made the ReSpec issue disappear.

Let's try to look into after merging...

zolkis · 2018-02-13T15:29:49Z

This is merged now. Thank you all for the comments and reviews.

zolkis added 3 commits January 29, 2018 13:53

Update the API according to discussion in w3c#82 and w3c#78

48bb879

Signed-off-by: Zoltan Kis <[email protected]>

Update use cases. Add callback types to Observables.

a814c2b

Signed-off-by: Zoltan Kis <[email protected]>

Change expose() to produce(), to reflect creation, not exposure of Ex…

b18f949

…posedThings. See also w3c#64. Signed-off-by: Zoltan Kis <[email protected]>

zolkis force-pushed the master branch from 59cc058 to b18f949 Compare January 30, 2018 12:40

Update the WoT API and the definitions; add SemanticAnnotations, Valu…

abe5714

…eDefinition, request handlers for ExposedThing Signed-off-by: Zoltan Kis <[email protected]>

Editorial pass to address review comments.

36d65a6

Signed-off-by: Zoltan Kis <[email protected]>

Remove ConsumedThing from ThingModel. ExposedThing implements Consume…

0f8724e

…dThing. Signed-off-by: Zoltan Kis <[email protected]>

Update examples. Update handler signatures for supporting generic han…

61dcf59

…dlers. Signed-off-by: Zoltan Kis <[email protected]>

zolkis added 2 commits February 5, 2018 13:53

Editorial fix for ReSpec errors

351de37

Signed-off-by: Zoltan Kis <[email protected]>

Add section for Observables. Remove optionality from some arguments a…

45d0598

…nd dictionary members. Signed-off-by: Zoltan Kis <[email protected]>

Add explanations for ExposedThing handlers.

144e580

Signed-off-by: Zoltan Kis <[email protected]>

zolkis force-pushed the master branch 2 times, most recently from ed6e690 to c69afdf Compare February 7, 2018 13:47

zolkis force-pushed the master branch from c69afdf to 688b0ab Compare February 7, 2018 14:19

zolkis changed the title ~~[WiP] Update the API according to discussion in #82 and #78~~ Update the API according to discussion in #82 and #78 Feb 7, 2018

zolkis requested review from danielpeintner and knimura February 8, 2018 08:38

Editorial pass: examples, use cases, notes, various small clarificati…

ba217f6

…ons. Removed duplicated statements. Signed-off-by: Zoltan Kis <[email protected]>

zolkis force-pushed the master branch from eb0a9eb to ba217f6 Compare February 9, 2018 08:49

w3c deleted a comment from zolkis Feb 12, 2018

danielpeintner approved these changes Feb 12, 2018

View reviewed changes

Fix review comments: examples, TD alignment etc.

2bfe65f

Signed-off-by: Zoltan Kis <[email protected]>

zolkis force-pushed the master branch from efec652 to 2bfe65f Compare February 12, 2018 12:04

Addressed review comments. Changed ValueType to DataSchema. Clarified…

ef36afb

… discovery constraint matching. Signed-off-by: Zoltan Kis <[email protected]>

zolkis merged commit 45666df into w3c:master Feb 13, 2018

		@@ -962,31 +837,46 @@ <h2>Examples</h2>
		<pre class="example highlight" title="Create a new blank exposed Thing">
		WoT.createExposedThing(thingDescription)

Update the API according to discussion in #82 and #78 #86

Update the API according to discussion in #82 and #78 #86

Uh oh!

Conversation

zolkis commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkovatsc commented Jan 31, 2018

Uh oh!

zolkis commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkovatsc commented Jan 31, 2018

Uh oh!

mkovatsc commented Jan 31, 2018

Uh oh!

zolkis commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkovatsc commented Jan 31, 2018

Uh oh!

knimura commented Feb 2, 2018

Uh oh!

zolkis commented Feb 2, 2018

Uh oh!

zolkis commented Feb 5, 2018

Uh oh!

zolkis commented Feb 5, 2018

Uh oh!

danielpeintner commented Feb 7, 2018

Uh oh!

zolkis commented Feb 7, 2018

Uh oh!

mkovatsc commented Feb 7, 2018

Uh oh!

zolkis commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkovatsc commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkovatsc commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zolkis commented Feb 7, 2018

Uh oh!

danielpeintner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zolkis commented Jan 29, 2018 •

edited

Loading

zolkis commented Jan 31, 2018 •

edited

Loading

zolkis commented Jan 31, 2018 •

edited

Loading

zolkis commented Feb 7, 2018 •

edited

Loading

mkovatsc commented Feb 7, 2018 •

edited

Loading

mkovatsc commented Feb 7, 2018 •

edited

Loading