-
Notifications
You must be signed in to change notification settings - Fork 662
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 support for functions.* (complete) methods #1702
Conversation
packages/web-api/src/methods.ts
Outdated
|
||
export interface FunctionsCompleteSuccessArguments extends WebAPICallOptions, TokenOverridable { | ||
function_execution_id: string; | ||
outputs?: Record<string, unknown>; |
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 believe this paramter is required (our work-in-progress document says so too)
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.
You're probably right. I'll retest to be doubly certain and remove optionality upon confirmation.
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.
Although, to be fair, outputs
may not be passed on so it's odd to require them!
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.
if i remember correctly, an empty outputs
object might be required even when the function does not have any required output_parameters in its definition.
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.
meaning the server-side can return an error code to such a request
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.
Indeed, on the deno side, outputs
is a required property (see this code).
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.
Thanks for tackling this! Yep, Kaz is right, outputs
is required when marking a function as complete.
packages/web-api/src/methods.ts
Outdated
|
||
export interface FunctionsCompleteSuccessArguments extends WebAPICallOptions, TokenOverridable { | ||
function_execution_id: string; | ||
outputs?: Record<string, unknown>; |
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.
Indeed, on the deno side, outputs
is a required property (see this code).
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.
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.
Thanks for working on this; now this looks good to me 👍
Summary
Adds support for the following
functions.*
methods:functions.completeError
functions.completeSuccess
Requirements (place an
x
in each[ ]
)