-
-
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
Shared concurrent heartbeat() promise #1026
Conversation
All concurrent heartbeat() calls will return the same promise object.
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.
@t-d-d thanks a lot for the PR. I left some comments. They are basically suggestions; the only one that I would like to see changed is making the utility a little less dense, so it's easier to grasp.
src/utils/sharedPromiseTo.js
Outdated
* @param { () => Promise<T> } [defaultFunc] | ||
* @returns { (func?: () => Promise<T>) => Promise<T> } | ||
*/ | ||
module.exports = defaultFunc => { |
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 we need defaultFunc
? It's currently not used anywhere. I would also use curly braces and make this a little bit less dense, 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.
I think you are right - it is trying to be too clever. I had both options because it's slightly easier to use the 'invocation time' method from a class (well, less code changes are needed to adopt it.)
I have changed to only provide option at initialisation time. Let me know what you think.
Actually this shouldn't be merged until we conclude the issue of what to do with failed heartbeats that @Nevon raises in #1025 . |
All concurrent heartbeat() calls will return the same promise object. Proposed solution to #1025
Added a test that reproduces #1025 but more tests needed.