-
Notifications
You must be signed in to change notification settings - Fork 358
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 Event Samples #151
base: master
Are you sure you want to change the base?
Add Event Samples #151
Conversation
throw e; | ||
} | ||
}); | ||
return 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.
response
will always be {}
because it's being set within a step.run
. Our execution model "enters" the function multiple times, with the last entrance skipping the step.run
callback and immediately returning its memoized data.
Here's a simple repro of that behavior:
inngest.createFunction(
{ id: "fn-1" },
{ event: "event-1" },
async ({ step }) => {
let response;
await step.run("a", async () => {
return (response = 1);
});
// Always prints undefined.
console.log(response);
}
);
We recommend setting response
to the return value of the step.run
:
inngest.createFunction(
{ id: "fn-1" },
{ event: "event-1" },
async ({ step }) => {
const response = await step.run("a", async () => {
return 1;
});
// Prints 1.
console.log(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.
@amh4r please take a look, I believe I have fixed this now, thanks for finding it!
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 actually pointless to use the step.run here? I originally had two steps, but now that I'm just doing one thing, it seems like extra code for now value. Should I just eliminate it?
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, in this case there isn't a need for step.run
since the whole function is the step.run
if (e instanceof Error) { | ||
console.error("Failed to process event", e, event); | ||
console.error(e.message); | ||
} else { | ||
console.error("Failed to process event for unknown reason", e, 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.
This works, though will log on each attempt. Is that your desired behavior, or would you rather only log if retries are exhausted? For example, would you want to see an error log if this step.run
retried twice and ultimately succeeded? There are valid reasons for either approach!
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 think I would want to log each time to catch transient errors. Thanks for the thought though, I hadn't considered that.
We want to add the event samples here for now until we find a more permanent location for them. The event platform beta goes out on Feb 3, 2025