-
Notifications
You must be signed in to change notification settings - Fork 158
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
Make 'Function' a generic type #203
Conversation
@lukehoban can you take a look? |
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.
LGTM
/** | ||
* Handler is the signature for a callback that be converted into an aws lambda entrypoint. | ||
*/ | ||
export type Handler<TArg, TResult> = (event: TArg, context: Context, callback: (error: any, result: TResult) => void) => any; |
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.
Is this TFoo
naming style common in TypeScript - I feel like I don't see it much.
Separately, perhaps the first type param should be Event
not `Arg?
Is Handler<E, R>
too succinct?
readonly identity: any; | ||
readonly clientContext: any; | ||
getRemainingTimeInMillis(): string; | ||
} |
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'm not positive - but I think we may have dependencies on this type from @pulumi/cloud
. We could choose to leave it here to avoid breaking clients (as an alias for the type in the other namespace), or could just assume that these changes you are making require a version bump to avoid getting automatically picked up.
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.
yes. tehse should go with a version bump.
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.
talking to matt about this.
This reverts commit dfb423e.
Followup to #202