-
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
Extended return type of invokeAction() #561
base: main
Are you sure you want to change the base?
Conversation
<p> | ||
The <dfn>error</dfn> property represents a possible error, initially `null`. | ||
</p> | ||
<p class="ednote" title="Should state be a function or an attribute only?"> |
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.
I started with a function but I start thinking an attribute should be sufficient..
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.
An action is supposed to be always polled? Or could a script subscribe just for changes in status?
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.
I think observing state changes would be nice but I am not sure whether that is actually necessary...
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.
Mhh, I tend to think now that a a function for reporting the status
would be more powerful and useful. What do people think? Other ideas?
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.
Mhh, I tend to think now that a a function for reporting the status would be more powerful and useful. What do people think? Other ideas?
Hmm, if I see it correctly, we could also treat the attribute as a getter and add more logic to that if needed, right?
Or could a script subscribe just for changes in status?
If we wanted to that, what would be the best way to model that? Adding a way for registering a callback function?
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.
Mhh, maybe we should decide what we need/want from the status
- is it likely that requesting the status may take a while? (or put differently do we need a Promise)
- do we need a way to observe a status change (would be nice but adds complexity)
My take is that having a promise function for status()
is fine while I am not 100% sure we should allow for listening status changes...
Are there opinions/proposals?
index.html
Outdated
"success", /* Profile uses completed? */ | ||
"error" /* Profile uses failed? */ |
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.
Should we coordinate in some way with the Profile TF to align the terms that are being used? Or is it no problem if they differ, since they are operating on a different level anyway?
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.
Good point. Once we feel comfortable with our choices we should definitely reach out. Most of the status terms are 1:1 matches. What seems different is pending
.
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.
FYI: Profile has 4 terms
pending
--> we don't have a corresponding term!running
-> same ✅completed
-> we usesuccess
insteadfailed
-> we useerror
instead
What can we do with aligning the terms? Use the ones from profile or talk to them? Any 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.
Hmm, their terminology actually sounds quite good to me, so from my side we could also adopt what they are using.
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.
I am fine withe the Profile terms also. The question remains if it is fine dropping "pending" then... (same same but different 🙃)
Other opinions?
We have some inline discussions which need some more feedback. Please chime in with your opinion! |
<p> | ||
The <dfn>error</dfn> property represents a possible error, initially `null`. | ||
</p> | ||
<p class="ednote" title="Should state be a function or an attribute only?"> |
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.
Mhh, I tend to think now that a a function for reporting the status would be more powerful and useful. What do people think? Other ideas?
Hmm, if I see it correctly, we could also treat the attribute as a getter and add more logic to that if needed, right?
Or could a script subscribe just for changes in status?
If we wanted to that, what would be the best way to model that? Adding a way for registering a callback function?
index.html
Outdated
"success", /* Profile uses completed? */ | ||
"error" /* Profile uses failed? */ |
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.
Hmm, their terminology actually sounds quite good to me, so from my side we could also adopt what they are using.
Co-authored-by: Jan Romann <[email protected]>
Scripting Call 2024/12/11
interface ActionInteractionOutput extends InteractionOutput, EventEmitter {
// cancel a pending/running action
cancel(): Promise<void>
// possible events
'pending': () => void;
'running': () => void;
'completed': () => void;
'failed': (error: Error) => void;
'cancelled': () => void;
// Note: value() function will only work after completed state has been reached
} Please comment! |
Nowadays the use of Promises is encouraged instead of events wherever possible (which tend to be misused). Promise are also easier to use from client code. I liked more the original design with status query returning a Promise. The latest status terms look good. |
Cancelling operations should be done using AbortSignal, but in our case a dedicated function is fine, since it's scope is strictly the referred action. |
@zolkis Thanks for your feedback!
I may be wrong in my thinking, but I think from a developer point of use, events seem simpler.
Note: The only thing I am not sure about is whether we can miss events. Why am I saying that.
I think it might be possible that the emit() call comes before Í registered the handlers? Anyhow, in the case of a status() promise I need to pull regularly, don't I?
I don't think there is a easy way to register for an event? |
We won't miss events, JS is single threaded, the event loop will check for events and dispatch. In my latest experience, devs preferred Promises to events, no setup is needed, and most interactions can be modeled with one-off transactions. What are the main dev use cases with this API?
|
Belongs to the <a>WoT Consumer</a> conformance class. | ||
An {{ActionInteractionOutput}} object is always created by a consumer implementation | ||
and exposes functionality to interact with long running (asynchronous) actions.<br /> | ||
Note: a synchronous action MAY not offer any <emph>additional</emph> sensible interaction beyond the ones from {{InteractionOutput}}. |
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
**MAY** not offer
is likely to be confused with**MAY NOT** offer
, despite formatting, casing, etc. - Further, I'm not sure what
sensible
is contributing here. - I'm also left wondering whether there should be some indication of what other
interactions
(sensible
or not) might be offered (if theaction
does offer more than those in{{InteractionOutput}}
)?
Perhaps this rephrasing is acceptable, for the first two points above? Something additional might be needed for the last.
Note: a synchronous action MAY not offer any <emph>additional</emph> sensible interaction beyond the ones from {{InteractionOutput}}. | |
Note: A synchronous action MAY offer only the interactions found in {{InteractionOutput}} and no others. |
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.
I've tried to cover the third aspect that you've mentioned by rephrasing the sentence as follows – hope that it is a bit clearer than it was before:
Note: a synchronous action MAY not offer any <emph>additional</emph> sensible interaction beyond the ones from {{InteractionOutput}}. | |
Note: The output of a synchronous action MAY be limited to the functionality of a regular {{InteractionOutput}} object, e.g., invoking the `cancel()` method might not have an effect. |
Should we allow pause/resume also? TD has no notion of it. | ||
</p> | ||
<section><h3>The <dfn>status()</dfn> function</h3> | ||
Reports the status of an <a>Action</a> (one of <code>running</code>, <code>success</code> or <code>error</code>) or rejects on error. The method MUST run the following steps: |
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.
Reports the status of an <a>Action</a> (one of <code>running</code>, <code>success</code> or <code>error</code>) or rejects on error. The method MUST run the following steps: | |
Reports the status of an <a>Action</a> (one of <code>running</code>, <code>success</code>, or <code>error</code>), or rejects on error. The method MUST run the following steps: |
I started to work on a PR to show a possible solution how the extended return type
ActionInteractionOutput
forinvokeAction()
may look like.resolves #555
Preview | Diff