fix(scheduler): prevent unwanted clearInterval#3044
fix(scheduler): prevent unwanted clearInterval#3044benlesh merged 2 commits intoReactiveX:stablefrom
Conversation
Generated by 🚫 dangerJS |
---3.6KB-green.svg?style=flat-square){kind=icon}
---0.1KB-green.svg?style=flat-square){kind=icon}
cartant
changed the title
cartant
changed the title
|
interesting. @cartant, can you please add a test? |
|
@benlesh Added two tests. |
benlesh
left a comment
benlesh
left a comment
There was a problem hiding this comment.
Minor nit about the wording of the test.
| sandbox.restore(); | ||
| }); | ||
|
|
||
| it('should recycle the interval for recursively scheduled actions with the same delay', () => { |
There was a problem hiding this comment.
Nit: This should probably say "reuse" rather than "recycle"... To me, "recycle" means you're tearing it down and creating another. Which is the opposite of the meaning here.
|
I'm going to have this target the 5.5 branch and cherry pick it over. We can do a patch. |
|
Actually, @cartant, do you think you can rebase your changes against |
|
@benlesh Rebased and tests renamed as per request. |
|
Hmmm... it looks like there are some TS 2.0 incompatible lines in this already... I'm not sure I can merge this into the stable branch. :( |
|
I can make the tests build on a clean const stubSetInterval = (<any> sinon.stub(global, 'setInterval')).callThrough();If you're okay with that, I can update the PR. I did look briefly at installing new typings, but the |
In AsyncAction, this.pending was assigned before the call to recycleAsyncId. That prevented the interval from being re-used resulting in the interval being cleared and re-created for each notification. Closes ReactiveX#3042
|
@benlesh I've made the change mentioned in the above comment and my branch's Travis build now passes - although, I had to clear the cache. |
|
LGTM 👍 |
|
@benlesh should be good now. |
fix: Revert "fix(scheduler): prevent unwanted clearInterval (#3044)"
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
6 participants
Description:
In
AsyncAction,this.pendingwas assigned before the call torecycleAsyncId. That prevented the interval from being re-used resulting in the interval being cleared and re-created for each notification.I cannot see how a test could be easily written for this, but the logic is straightforward.Related issue (if exists): #3042