-
-
Notifications
You must be signed in to change notification settings - Fork 527
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
Define the options for the runner constructor #843
Define the options for the runner constructor #843
Conversation
a43b9ba
to
57b70c4
Compare
src/consumer/runner.js
Outdated
* @param {(payload: import("../../types").EachBatchPayload) => Promise<void>} options.eachBatch | ||
* @param {(payload: import("../../types").EachMessagePayload) => Promise<void>} options.eachMessage | ||
* @param {number} [options.heartbeatInterval] | ||
* @param {(reason: any) => PromiseLike<never>} [options.onCrash] |
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.
Pretty sure reason
should be Error
.
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.
Good one. I think I copied that one from the suggestions, and didn't do much checking. Thinking about it further we probably should define what we expect a crash handler to do:
- Setting the parameter to
Error
seems reasonable, with the minuscule risk that somewhere deep down the stack some misbehaved library rejects a promise with a non-error thing. - It seems that the return value of onCrash handler is never looked at, so maybe
void
is enough?
@param {(reason: Error) => void} [options.onCrash]
WDYT?
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.
Actually, look at scheduleJoin
where we use it in a .catch
, meaning it should return a PromiseLike
, right?
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.
Based on the types in lib.es5.d.ts: "Anything goes, really":
catch<TResult = never>(onrejected?: ((reason: any) => TResult | PromiseLike<TResult>) | undefined | null): Promise<T | TResult>;
I do notice that onCrash isn't optional though: in scheduleJoin
undefined would be ok, but in start
just below we call it unconditionally.
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.
"Anything goes, really"
That should be the slogan for Javascript 😄
I do notice that onCrash isn't optional though: in scheduleJoin undefined would be ok, but in start just below we call it unconditionally.
Right, yes, it should not be optional.
* Drop the return value: We don't do anything with it anyways, so the provider of that handler shouldn't make any assumptions * Refine the `reason` parameter to always be an 'Error' instance, and accept the minimal risk that some downstream library does stupid things * Make the `onCrash` property non-optional, as we do refer to ith without a guard
This is only relevant when developing, as it provides more type information (and incidentally makes it possible to ctrl-click on things to navigate! :D)