Skip to content

Update WoT runtime, discovery, ExposedThing, ConsumedThing. #21

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

Merged
merged 14 commits into from
May 2, 2017

Conversation

zolkis
Copy link
Contributor

@zolkis zolkis commented Apr 10, 2017

Update the spec on text relevant on the following topics:

@zolkis zolkis force-pushed the master branch 2 times, most recently from c2399a3 to 6e04422 Compare April 10, 2017 10:34
@zolkis zolkis changed the title Update WoT runtime related text Update WoT runtime, discovery, ExposedThing, ConsumedThing. Apr 10, 2017
zolkis added 3 commits April 11, 2017 17:39
…me vs Applications vs Scripts

Signed-off-by: Zoltan Kis <[email protected]>
Signed-off-by: Zoltan Kis <[email protected]>
Signed-off-by: Zoltan Kis <[email protected]>
@zolkis
Copy link
Contributor Author

zolkis commented Apr 15, 2017

Based on discussion in #22 , removed the remote Thing management (subject to further debate).

@zolkis zolkis requested review from mkovatsc and h0ru5 April 18, 2017 08:28
index.html Outdated
</dl>
<p>
<dfn data-lt="consume|consume a TD|consuming a TD">Consuming a Thing Description</dfn> means parsing the <a>Thing Description</a> and building a resource model with the use of <a>Protocol Bindings</a> that can be used in a script for accessing and controlling the Thing. A <a>Thing</a> when starts up, consumes its own <a>TD</a>, then it exposes its <a>WoT interface</a> as a server. Note that a script on a <a>Thing</a> may consume other <a>TD</a>s and expose a combined interface of the consumed <a>Things</a>.
</p>
<p>
A <dfn>WoT Runtime</dfn> or <dfn>WR</dfn> is defined as a script execution environment that manages the lifecycle of WoT application scripts, implements a script interpreter, an event loop, a security enforcement point for access management and uses an operating system API that provides access to local and remote resources. A <a>WR</a> should be isolated from other execution environments on memory address space, storage address space (file system), network namespace, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

optional suggestion (no impediment for merge, I can open an own PR)

"...and uses an operating system API that provides access to local and remote resources."

replaced by:
"...and uses lower-level APIs to provide access to local and remote resources."

Rationale: the runtime itself may be sandboxed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, valid! Corrected.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@h0ru5 h0ru5 left a comment

Choose a reason for hiding this comment

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

I generally agree, but see the need for some technical detail changes/additions before the freeze. This might be easier to to on a merged & squashed master.

index.html Outdated
"propertychanged",
"propertyremoved",
"propertyadded",
"actionchanged"
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that these are "well-known" events and applications MAY define additional ones, right?
are "actionchanged" && "actionremoved" intentionally left out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean actionadded and actionremoved, since actionchanged is there.
Yes, there might be value for clients to know there is different action or no action from now, or a new action in play.
I can add them.
We could also parametrize events, i.e. have only propertychange and actionchange, then the event data will carry the information whether was that adding, removing or updating.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, sorry, typoed.
the parametrized event makes sense for me, but I'd think it could even lead to "interfacechanged" (or maybe "TDchanged") with the parameter like {"method" : "add", "type" : "action", "name" : "bla"}

Not sure which one is better: to have many simple events or fewer parameterized ones.

Copy link
Contributor Author

@zolkis zolkis Apr 25, 2017

Choose a reason for hiding this comment

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

I think it's simpler to write code for simple events. There is only 6 of them.

But I like the idea of having one event for descriptionchange. It is more expressive and creates less confusion (for instance with propertychange it is not clear whether the property value has changed, or the type, or the access). Of course we could have both propertychange (for TD) and propertyupdate or valuechange (for value). But it would be more clear to have one descriptionchange event for TD changes for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we'd either have

  1. propertychanged (value), descriptionchanged (TD) and actioninvoked,
  2. or a flat list of
    valuechanged (value),
    propertychanged, propertyadded, propertyremoved, actionadded, actionchanged, actionremoved (TD),
    actioninvoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 1. would be more efficient solution, because many TD changes could be expressed with one message, which also represents a TD change transaction, i.e. changes that needed to happen in the same time. Otherwise we cannot express this concept. So for the moment I will add the first event list.

index.html Outdated
callback ActionHandler = void (ThingAction action, USVString url);
callback ObserveHandler = void (DOMString eventName, USVString url);
callback PropertyHandler = void (ThingProperty property, USVString from);
callback ActionHandler = void (ThingAction action, USVString from);
Copy link
Contributor

@h0ru5 h0ru5 Apr 25, 2017

Choose a reason for hiding this comment

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

Actions have parameters and return values (inputData & outputData in the TD).
Those can be null.

Therefore (hope the WebIDL is valid):
callback ActionHandler = Any (ThingAction action, USVString from, Any object param);

regarding the return I am trying to express: an equivalent to TypeScript: "any | void" - in the function can be any return, but the compiler allows you to go without stating return in the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

correction: It should be Any instead of object for the param and the return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had slightly different understanding on how this works. Which use case do we implement here:

  1. notify all observing clients if an action was called
  2. notify all observing clients if an action was called, including which parameters was it called with and what was the return value?

Currently 1. is supported and you argue for supporting 2. It's fine if we have evidence this is a required use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, Ok. I interpreted it as (3) setting the callback that gets called when the action is invoked.

Copy link
Contributor

Choose a reason for hiding this comment

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

from those cases i'd clearly opt for (1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I messed it up. Disregard my previous comment. This is on the ExposedThing, and we are discussing the request handler for action related requests. The use case is that a request comes in from a client to invoke a local method. The ThingAction parameter contains all action related data: name, parameters, return value etc. The current WebIDL seems to be correct to me.

index.html Outdated
// define request handlers (one per request type, so no events here)
ExposedThing onRetrieveProperty(PropertyHandler handler);
ExposedThing onAddProperty(PropertyHandler handler);
ExposedThing onRemoveProperty(PropertyHandler handler);
Copy link
Contributor

Choose a reason for hiding this comment

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

+ ExposedThing onWriteProperty(PropertyWriteHandler handler);

This hook allows to react if somebody updates the property

Copy link
Contributor

Choose a reason for hiding this comment

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

+ callback PropertyWriteHandler = void (ThingProperty property, USVString from, optional Any oldValue, optional Any newValue);

As suggestion - the desgin rationale is the $watch callback from Angular.js (v1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed that is missing. I'd prefer the names onUpdateProperty and PropertyUpdateHandler since it matches better CRUDN terminology.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, right. I think we even had been naming it onUpdateProperty in current practices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we don't need a separate PropertyUpdateHandler since in the request the ThingProperty parameter does have a requested new value, and the old value is what it is now, that is not changed. This is a request handler, not an event for notifications.

rationale.md Outdated


Resolutions:
- The WoT API uses a namespace object `wot` in the browser, or provided by `require()` in standalone runtimes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I understood the comment in this way: as an application developer, I want to be able to rely on the namespace to be magically there on global. If we want to give the option to explicitly require it, we can add the option that developers should be able to require (common.js) / "import" (ES6) it.

If you agree, I would suggest the following formulation:
- It is a runtime implementers' design choice whether the WoT API is accessible for application developers via a global namespace `wot` ( as usual in the e.g. browser), or by `require()` resp. `import` (as usual e.g. in standalone runtimes.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the browser it shouldn't really be a choice. For runtimes, are you saying it can also be a namespace or global object? (plus obtainable by require or import)?

Copy link
Contributor

@h0ru5 h0ru5 Apr 25, 2017

Choose a reason for hiding this comment

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

I mean the for the interface between non-browser runtimes and their application the choice is: (a) like in the browser (b) require/import it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree and this is what I meant in the text. So we need to make this more clear. What about this:

The WoT API uses a namespace object wot in the browser. In non-browser runtimes, implementations may use either a namespace object wot, or an API object provided by the require() or import() mechanisms.

index.html Outdated
};

callback ThingDiscoveryCallback = void (ConsumedThing thing);
enum DiscoveryType { "any", "local", "directory", "broadcast" };
Copy link
Contributor

Choose a reason for hiding this comment

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

Bear with my limited knowledge of WebIDL: is this enum extensible?

If not so, I'd suggest to have a list of well-known types, but the possibility to add any string here.
This is an important extension point IMO, as there are other discovery methods (e.g. reactive, such as geo, eddystone/physical web, ...) we can pick up work from the discover TF to fill the well-known list, but I suggest we leave it extensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, enums are not extensible, if we wanted that would be just using a String.
However, this was intentional, as the supported discovery types should be standardized, since they require explicit and well defined support from the underlying platform and depend on well known standards.
There is no problem adding them to the spec, but they should be explicitly specified. Subsequent versions of the spec can extend the enum with new values.
Otherwise the API would be like discover({ type: "app-specific-opaque-string"}); which is more like a free text search than scoping for supported discovery types.

Copy link
Contributor

Choose a reason for hiding this comment

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

The risk for an application devloper is: "if I use a vendor-specific non-standard discovery method it will fail on a runtime that does not support this."

I see exactly this extensibility as a feature - have a well-defined of standardized discovery methods, but allow vendors to add own discovery methods that can later be standardized. That's how the browsers do & did evolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In JavaScript they of course get mapped as String. The question is how do we describe in the algorithm of the discover() method. I prefer keeping (and extending now) the enum, eventually include an option for extended type but described in the algorithm.
We should cover the known discovery types in this version. BLE and NFC could be covered by "nearby". I think that should be enough since usually one method is used and the purpose of the filter is to rule out the others (e.g. broadcast type).

index.html Outdated
<p class="note">
The WebIDL interfaces presented in this initial version of the document are based on the [[!WOT-PRACTICES]]. It is expected to be updated soon.
Browser implementations SHOULD use a namespace object such as `wot`, and [Node.js](https://nodejs.org/en/)-like runtimes MAY provide the API object through the [`require()`](https://nodejs.org/api/modules.html) mechanism.
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment below. Rationale:

  • can be global namespace for all kind of runtimes
  • design decision for implrmrmnter
  • ES2015 import additionally to require()

Suggestion:
Browser implementations SHOULD use a namespace object such as `wot`, and [Node.js](https://nodejs.org/en/)-like runtimes MAY alternatively provide the API object through the [`require()`] (https://nodejs.org/api/modules.html) or [`import`](http://www.ecma-international.org/ecma-262/6.0/#sec-imports) mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Adding.

@zolkis
Copy link
Contributor Author

zolkis commented Apr 25, 2017

Please check in the source the lines marked with TBD.
There've been a lot of "consolidating" changes in the last commits, and these are the opens.

zolkis added 3 commits April 26, 2017 09:31
… in local runtimes. Remove semantic translators. Remove handlers for adding and removing property. Add options to property requests. Add discovery type for 'other' extensions. Add references to other WoT WG specifications.

Signed-off-by: Zoltan Kis <[email protected]>
h0ru5 and others added 2 commits April 28, 2017 09:56
@h0ru5
Copy link
Contributor

h0ru5 commented May 2, 2017

As discussed among the editors, we will merge this PR to create a state that can be the base for the F2F freeze resp. new detailed PRs if needed.

@h0ru5 h0ru5 merged commit e432a3a into w3c:master May 2, 2017
@h0ru5
Copy link
Contributor

h0ru5 commented May 2, 2017

Merged as discussed in the webconf on 02-05-2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants