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

Return event IDs when sending events #195

Closed
wants to merge 3 commits into from

Conversation

jpwilliams
Copy link
Member

Summary

When events are sent to Inngest, return the event IDs from the response.

@jpwilliams jpwilliams self-assigned this May 13, 2023
@changeset-bot
Copy link

changeset-bot bot commented May 13, 2023

🦋 Changeset detected

Latest commit: 361d7fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
inngest Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@djfarrelly djfarrelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jpwilliams - this looks great! The only question that I have on this one is whether we want the return value to be just the event IDs, or is there a possibility in the future to add more data to the response in which case we may want to return an object? e.g.

const { ids } = await inngest.send([...])

I don't currently have any other ideas, but before shipping, I might challenge us to ask the team to think if there are any other ideas in which we might want to pass them through to the client.

json: () => Promise.resolve({}),
json: () =>
Promise.resolve({
ids: Array.isArray(input) ? input.map((_, i) => `${i}`) : ["0"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this and the test below, I noticed we may not have a test w/ an array of multiple events, but we do have one w/ an array. Is that something we should cover?

ids: z.array(z.string()),
status: z.number().min(200).max(299),
})
.parseAsync(rawBody);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide at some point to return more data from the response and the shape of the response changes, would this parsing fail? Or will it only parse the data expected and ignore the rest?

Copy link
Member

@djfarrelly djfarrelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second review after reviewing the middleware docs @jpwilliams, I'm kind of leaning towards returning the entire response here instead of just the string. We may return other metadata and just returning a string array limits us in the future instead of just returning an object.

Copy link
Member

@djfarrelly djfarrelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further thought - we definitely should return an object for future extensibility. I'll make that change on this branch after rebasing from main.

@jpwilliams
Copy link
Member Author

Closing for #256.

@jpwilliams jpwilliams closed this Jul 27, 2023
jpwilliams added a commit that referenced this pull request Sep 25, 2023
## Summary

We should return the response data from the Event API, including `ids`
when events are sent. This returns an object for future extensibility
beyond `ids`.

Applies the changes in #195, but in a post-2.0, post-Middleware setup.

- [x] Add return data to `inngest.send()`
- [ ] Add return data to `steps.sendEvent()`

---------

Co-authored-by: Jack Williams <1736957+jpwilliams@users.noreply.github.com>
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.

None yet

2 participants