-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add ExposedThing property observe, unobserve and event handler support #218
Conversation
Needs more work. This commit is quite old and since then there has been development on how to use |
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
@danielpeintner, @relu91, @egekorkan the last commit is a substantial update on ExposedThing algorithms taking into account the latest developments in ConsumedThing. Please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general comment, I think we should wait until we clarify the issue #219. I made some points throughout the review, but no major revision is needed. Great job! 👍
index.html
Outdated
A function that is called when an external request for reading a <a>Property</a> is received and defines what to do with such requests. It returns a {{Promise}} and resolves with an {{InteractionData}} object | ||
created by running the <a>create interaction data</a> steps on the | ||
value of the <a>Property</a> matching the |name:string| argument that is obtained, or rejects with an error if the property is not found or the value cannot be retrieved. | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment may be applied also to the other handlers. I think we can move the complexity of creating interaction data from the application to the runtime. If the read handler returns any
then the runtime can execute the create interaction data
algorithm on that object. Application will still be able to represent payloads that does not have data schema (i.e. raw images) by returning InteractionData
objects. On the other hand it could simply return a valid javascript value (i.e. true, an object etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I should not summarize an algorithm in that paragraph, just say what does it return.
It is the algorithmic steps that should specify what to do. Removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but I actually meant is that the promise should return any
. So that the application does not need to worry about how to create an InteractionData
in trivial cases. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have agreed in the calls that reading functions always return InteractionData
that has the value()
convenience function and is created by the impl, while for writing functions we accept any
, in order to relieve apps from creating InteractionData
for trivial cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That of course on ConsumedThing side.
On the ExposedThing side, the handlers also return InteractionData
, and - now I get the question :) - you're asking why not allowing to return any
in the handler functions and let the implementation bother with InteractionData
.
Well, a handler provides total control - an app may actually want to define how to create InteractionData
. In fact you're right, we could apply the same principle we applied at ConsumedThing writes, i.e. allow returning any
, and if that is an InteractionData
, impls should just use it as it is, otherwise should use whatever is returned in order to construct a reply, possibly involving a Body-like object (which is InteractionData-like).
So all-in-all I think this is fair. I will modify the handler signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is consistent: when data comes from the implementation to the script, it is InteractionData
, except eventually for Actions (discussed here) where we'd use any
for the reasons explained above.
When data comes from scripts to implementation, we use any
.
IMHO this is the best way and most developer friendly.
The algorithms, notes and examples should provide clarity on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, apps can just pass values like 5
or five
instead of being required to create streams and InteractionData
out of those.
That also applies for ExposedThing handlers. The implementation can work fine with the values provided by the script, following the algorithms and can create streams if necessary. However, apps do have the options to override that by providing InteractionData
themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can provide a constructor to InteractionData
that takes any
data as input, and use InteractionData
everywhere in the API. But that is one step more complex for apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@relu91 what's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on the call on 15.6.2020, we leave the current interfaces and include an explanatory section on how InteractionData
vs any
are used.
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Thanks @relu91 for the meaningful comments. I have made some fixes, and explained the rest. |
I replied to some comments all the other are resolved, however it seems that I cannot mark them as resolved because I do not have the write access (see here) . It is funny that I made the review but I cannot mark it as resolved 😄 |
…lgorithms and add notes. Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Huhh. Now it should be symmetric wrt InteractionData between ConsumedThing and ExposedThing, provided I didn't mess it up :). Maybe needs a re-read :). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one last change and I think it is ok!
index.html
Outdated
optional InteractionOptions options = null); | ||
|
||
callback ActionHandler = Promise<InteractionData>(any params, | ||
callback ActionHandler = Promise<any>(any params, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So following this argument:
Done. Actually it's the exact opposite of how we do it in ConsumedThing.
When data comes from the implementation, it's InteractionData, when it goes there, it's any.
Shouldn't params be InteractionData
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that is tricky. In ConsumedThing the params can be a single value, array or an object (used for multiple parameters).
Here in an action handler we may get a single value, array, InteractionData
, or an object with {simple values, array, InteractionData
or other objects}, recursively.
This part is simply underspecified - please make suggestions how to handle it. Maybe we can fix that in a separate PR as well.
If we use InteractionData
, all of the parsing falls on the app, which may be OK, but that functionality is already implemented in the ConsumedThing implementation underneath, so we'd force the app to do something that's also done by the impl - in which case why should an app bother using a WoT Scripting implementation, just make a shim over a comms lib.
If we use any
, perhaps we should make a note about what to expect, and specify an algorithm on what to do when an action is invoked - it will be similar to the write steps. This seems to be the more developer-friendly option, so I'd prefer this.
The TODO is then not changing the signature, but completing the algorithm on action requests, if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that is tricky. In ConsumedThing the params can be a single value, array or an object (used for multiple parameters).
Maybe I miss this: How can we specify different parameters on the TD? As far as I know, we have only the input
property to specify the Dataschema of the input. In this sense, I do not see any difference with the writeHandler
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TD spec has always had trouble with multiplicity :). The idea was that one input
is enough since it can describe multiple inputs as an object, and it can also describe a single param just as a single param. It would have been more clear if input
was an array of DataSchema's, but it's this way now. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I admit that representing multiple params as an object is better since if all of them are optional, you can name the ones used, instead of filling the sig with nulls. A bit awkward, perhaps not well explained in the TD spec, but works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we move this discussion in a new issue/PR? it is fine to leave the PR as it is and change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I will fix the algorithm a bit later, maybe on Friday. We'll need to review it again it seems.
About multiple properties handling, it is underspecified now (it's implementation specific how do they tell it's a multiple properties request and how they deserialize the property names). We can handle that in another PR.
Anyway, it's better to comment in the PR than on the commits directly, since this discussion will be buried :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like after checking the TD, it is possible to use InteractionData
when passing args to the handler, which I did.
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
@danielpeintner @relu91 |
This PR became so huge that it does not diff any more by default, and it's anyway unusable. |
I agree
Hence, it might make sense to merge. BTW. Thanks for the figures. I believe this is useful for many people! I have 2 additional comments/ideas:
I think using the type
|
The observe handler is basically a read handler, so from one hand it would be misleading to name it
Nice finding. Though what we need is In the end I am not sure what do we gain, since the role inversion in ConsumedThing vs ExposedThing messes things up, and then in the algorithms we cannot save naming the types what they are, I would leave this discussion for later PR and will fix the nits now. |
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Maybe we could try the following: typedef any DataSchema; // explained in algorithms
typedef (ReadableStream or DataSchema) InteractionInput; And rename |
We have another problem, the |
Another way would be to be able to construct |
Actually Should I update this PR with that, or should we merge this and make it in a separate PR? |
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
In the end I have updated this PR, since it belongs here. |
Again, here you can read the clean rendered version: |
I also think this is much better. The only thing that still bothers me are the terms "input" and "output" in e.g., emitEvent with *input does not "read" right to me. Also ActionHandler returnig I wonder whether we can find better names... |
Actually it makes sense, since The Promise returned from handlers has been discussed in the call and IIRC @relu91 requested these to be a Promise (easing server side programming). Made sense to me. I suggest we use these as working names and discuss it later. |
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
It was again about the name ... meaning that a return value is of type |
Again, it's not a bug, it's a feature :). |
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
5192c8e
to
7dd8175
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer I only read the diffs of the last commits since I already accepted the previous changes.
Great job @zolkis I think the new pictures and the renaming helps a lot to understand what is going on in the infrastructure. We did well to take time and review what we had done, thank you @danielpeintner for suggesting this👍 .
One last comment, as well about a naming issue (sorry 😃 ). The document define interactionInput
as DataSchema or ReadableStream
. In my opinion, the name DataSchema
is quite confusing (ok less than any). The problem is the clashing with DataSchema
defined in the TD spec. So one reader can interpret it as an instance of a DataSchema
and not a value that satisfies a DataSchema
(as correctly written later on in the document). Hence, if everyone agrees, I'll do one final change to this biblical PR: rename DataSchema
to DataSchemaValue
(or something better?) I guess we are finishing the names 😄 .
Yes, indeed I hesitated about |
bbb38af
to
90af40f
Compare
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
90af40f
to
13b1a24
Compare
Made some typos and amended the last commit twice. |
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Signed-off-by: Zoltan Kis zoltan.kis@intel.com
Preview | Diff