fix: ensure drained() resolves after async tasks complete#89
Conversation
| previousDrain() | ||
| resolve() | ||
| } | ||
| setImmediate(() => { |
There was a problem hiding this comment.
Could you check if a queueMicroTask() call does the job as well? I would prefer not to turn the event loop.
There was a problem hiding this comment.
Could you check if a queueMicroTask() call does the job as well? I would prefer not to turn the event loop.
That won't run on older versions of node though and I noticed in other PRs that you want it to be compatible all the way back essentially.
process.nextTick() does the job and is even higher priority than queueMicroTask(), so I can use that if you like? My worry was adding the ability to potentially starve the event loop, although it shouldn't be possible with normal usage.
Let me know how you'd like to proceed.
There was a problem hiding this comment.
nextTick is perfect here, thanks!
There was a problem hiding this comment.
Regarding starving the event loop, it is 100% possible with this module. Either it's a concern applied to all methods or it's not, and I prefer to leave that decision to the users.
There was a problem hiding this comment.
fixes #88