Skip to content
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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

danielpeintner
Copy link
Contributor

@danielpeintner danielpeintner commented Aug 26, 2024

I started to work on a PR to show a possible solution how the extended return type ActionInteractionOutput for invokeAction() may look like.

resolves #555


Preview | Diff

index.html Outdated Show resolved Hide resolved
<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?">
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 started with a function but I start thinking an attribute should be sufficient..

Copy link
Contributor

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?

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 observing state changes would be nice but I am not sure whether that is actually necessary...

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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?

@danielpeintner danielpeintner marked this pull request as draft August 26, 2024 14:20
index.html Outdated
Comment on lines 1295 to 1296
"success", /* Profile uses completed? */
"error" /* Profile uses failed? */
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 use success instead
  • failed -> we use error instead

What can we do with aligning the terms? Use the ones from profile or talk to them? Any opinion?

Copy link
Member

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.

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 am fine withe the Profile terms also. The question remains if it is fine dropping "pending" then... (same same but different 🙃)
Other opinions?

@danielpeintner
Copy link
Contributor Author

We have some inline discussions which need some more feedback. Please chime in with your opinion!

index.html Outdated Show resolved Hide resolved
<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?">
Copy link
Member

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
Comment on lines 1295 to 1296
"success", /* Profile uses completed? */
"error" /* Profile uses failed? */
Copy link
Member

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]>
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.

Extended return type of invokeAction()
3 participants