-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix not cleared storage task handlers from setImmediate #5016
Conversation
@cartant Why did the build fall? |
It's nothing that was done in this PR. The version of BTW, @mazulatas, I'll review your PR a little later. It looks good; there's just one or two things I'd like changed. Thanks. |
@cartant Thanks for review, we are waiting you |
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.
LGTM, I'd just like to suggest a couple of name changes.
src/internal/util/Immediate.ts
Outdated
@@ -21,3 +22,9 @@ export const Immediate = { | |||
delete tasksByHandle[handle]; | |||
}, | |||
}; | |||
|
|||
export const ForTests = { |
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'd suggest renaming ForTests
to TestTools
and renaming numberOfTasksToHandle
to pending
.
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 agree
- Reuses the same promise instead of allocating a new one on each turn. - Uses an array to track handles rather than an object. The array should stay sufficiently small, should reduce memory pressure, and should have more consistent performance characteristics over time. The object property lookups will end up becoming slower and slower as more keys are added to it.
Thank you @mazulatas for catching this one and bringing it to my attention. I've further refactored the code to make it a little more efficient than it was, and I've kept your test and test util. (I did the code editing in the browser, so we'll see if CI passes, haha) |
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.
LGTM
Registered handlers would sometimes leak in memory, this resolves that issue and adds a test. Related ReactiveX#5016
Registered handlers would sometimes leak in memory, this resolves that issue and adds a test. Related #5016
Description:
This PR adds cleaning up handle tasks in Immediate