-
Notifications
You must be signed in to change notification settings - Fork 3
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
Cleanup the Inngest's client send
interface
#69
Conversation
6c31189
to
fdddaea
Compare
fun send(events: Array<InngestEvent>): SendEventsResponse? { | ||
val request = httpClient.build("$baseUrl/e/$eventKey", events) | ||
|
||
return httpClient.send(request) lambda@{ response -> | ||
// TODO: Handle error case |
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 can handle the error case in: #57
Making it clearer that it supports sending multiple events at once.
fdddaea
to
0fc1ebf
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.
looks good, main question is on use of an array vs List
@@ -206,23 +206,22 @@ class Step( | |||
} | |||
|
|||
/** | |||
* Sends multiple events to Inngest | |||
* Sends a single event to Inngest. |
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.
nice catch on the docs here
* @param events The events to send. | ||
* | ||
*/ | ||
fun send(events: Array<InngestEvent>): SendEventsResponse? { |
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.
Hm, I can't find a concrete reference for this but I think Kotlin Array
is the equivalent of a java array, which means InngestEvent[]
which I think tends to be less flexible than the List
interface, since that has multiple implementations and []
arrays can easily be made into ArrayList
Some SO posts that seem to align with that I'm saying, but nothing definitive
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.
let's address this in a follow up
* @param event The event to send. | ||
* | ||
*/ | ||
fun send(event: InngestEvent): SendEventsResponse? = send(arrayOf(event)) |
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.
do other SDKs use the plural response type even if a singular event is sent?
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.
both do yeah.
@@ -21,37 +19,28 @@ class Inngest | |||
|
|||
internal val httpClient = HttpClient(RequestConfig(headers)) | |||
|
|||
inline fun <reified T> send(payload: Any): T? = |
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.
oh nice, we can get rid of the reified type
Summary
Cleanup the Inngest's client
send
interface, making it clearer that it supports sending multiple events at once.Changes:
sendEvent
method.send
method and cleanup the implementation: Removed the unnecessary generic andAny
typing.Checklist
Related