-
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
INN-2310: Implement step.sendEvent #21
Conversation
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
ktlint
inngest-test-server/src/main/kotlin/com/inngest/App.kt|56 col 11| Property name should use the screaming snake case notation when the value can not be changed
007eb3a
to
3eb4eb1
Compare
It has two signatures to handle both a single event or an array of events.
3eb4eb1
to
b229b33
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.
non blocking comments.
if you're not planning to address them in this PR, would be best to add some TODO
comments so it's clear what needs to be revisit later on.
fun sendEvent( | ||
id: String, | ||
events: Array<InngestEvent>, | ||
) { |
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.
the return type should be similar to send
itself.
https://www.inngest.com/docs/reference/functions/step-send-event#step-send-event-id-event-payload-event-payload-promise-ids-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.
gotcha, I'll address them in this PR 👍
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.
Turns out this is a bit tricky, we left a TODO to investigate later
return | ||
} catch (e: StateNotFound) { | ||
val response = CommHandler.sendEvent<SendEventsResponse>(events) | ||
throw StepInterruptSendEventException(id, hashedId, response!!.ids) |
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.
minor, let's see if we can change the return type of CommHandler.sendEvent
to guarantee non null response
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'll investigate this later too
Add support for
step.sendEvent
to send events out.Introduces a small refactor to the
CommHandler
http client to dry out API calls.