Add option to reset interval on success #18
Conversation
|
Ah, sorry. Very nice! Ben was just talking at ng-conf about reset for |
|
No worries! Let me know if you'd prefer to submit the unrelated commits separately to keep this pull-request cleaner. |
yarn.lock
Outdated
| @@ -2926,6 +2926,13 @@ run-queue@^1.0.0, run-queue@^1.0.3: | |||
| dependencies: | |||
| aproba "^1.1.1" | |||
|
|
|||
| rxjs@^6.4.0: | |||
There was a problem hiding this comment.
this would be reverted as well
src/operators/retryBackoff.ts
Outdated
| return <T>(source: Observable<T>) => | ||
| source.pipe( | ||
| return <T>(source: Observable<T>) => { | ||
| let index = 0; |
There was a problem hiding this comment.
This would create a problem with referential transparency.
Nicholas Jamieson describes it very well here: https://stackoverflow.com/a/52022545/1167879
There was a problem hiding this comment.
💯 Awesome catch and thanks for the link. Very interesting explanation of the perils of keeping state in operators. I also found this article, by the same author, which goes into a bit more details about the same problem.
These are the steps I took to fix this issue:
- add two tests related to this internal state problems (296637b): ✔️ Tests passing
This was to confirm everything worked as expected before my changes to the operator. - add my initial naive implementation (d1356ab): ❌ Tests failing
One of those two tests was failing, which confirms the initial implementation was buggy. Although it wasn't the referential transparency one that was failing, it was the other one. - apply the fix suggested in stackoverflow/linked article (df20de5): ✔️ Tests passing
There was a problem hiding this comment.
Very nice! Thanks for getting it done.
Let's adjust the tap and one more quick one - do you have prettier (extension) installed for formatting? I should've added it to dev deps as well 😕
Also, what are your thought about making resetOnSuccess = true by default?
There was a problem hiding this comment.
I've reformatted the retryBackoff.ts file using this prettier config (because it seemed close enough to the original formatting):
{
"arrowParens": "avoid",
"singleQuote": true
}Also, what are your thought about making resetOnSuccess = true by default?
My impression is that having it true by default is a reasonable default (I think more often than not that's what you want). My main concern is that it's a breaking change but that shouldn't be a problem if it will be introduced with a major version bump and documented accordingly.
src/operators/retryBackoff.ts
Outdated
| ) | ||
| ), | ||
| tap((_: T) => { | ||
| if (resetOnSuccess) { |
There was a problem hiding this comment.
if you are not using arguments, then there's no need to put them at all :) () => {...}
There was a problem hiding this comment.
Removing the argument completely makes TypeScript throw an error when compiling the code. Keeping the argument in there, even if it's not used, seemed to be a good trade-off in order to make the code compile.
I would still like to get rid of it as you've suggested but I haven't found a way to do so in a clean way. Let me know if you have any suggestions.
There was a problem hiding this comment.
Hm... I don't see any complains from TS 🙂(did a git pull for you PR).
There was a problem hiding this comment.
You're right, it seems to compile just fine now (8fa59d7). Not sure what has changed since I saw that error but it's good that it works now.
The mocha `--compilers` flag is deprecated in favor of `--require`: https://github.com/mochajs/mocha/wiki/compilers-deprecation
src/operators/retryBackoff.ts
Outdated
| ) | ||
| ), | ||
| tap((_: T) => { | ||
| if (resetOnSuccess) { |
There was a problem hiding this comment.
Removing the argument completely makes TypeScript throw an error when compiling the code. Keeping the argument in there, even if it's not used, seemed to be a good trade-off in order to make the code compile.
I would still like to get rid of it as you've suggested but I haven't found a way to do so in a clean way. Let me know if you have any suggestions.
src/operators/retryBackoff.ts
Outdated
| return <T>(source: Observable<T>) => | ||
| source.pipe( | ||
| return <T>(source: Observable<T>) => { | ||
| let index = 0; |
There was a problem hiding this comment.
💯 Awesome catch and thanks for the link. Very interesting explanation of the perils of keeping state in operators. I also found this article, by the same author, which goes into a bit more details about the same problem.
These are the steps I took to fix this issue:
- add two tests related to this internal state problems (296637b): ✔️ Tests passing
This was to confirm everything worked as expected before my changes to the operator. - add my initial naive implementation (d1356ab): ❌ Tests failing
One of those two tests was failing, which confirms the initial implementation was buggy. Although it wasn't the referential transparency one that was failing, it was the other one. - apply the fix suggested in stackoverflow/linked article (df20de5): ✔️ Tests passing
|
I've added two more commits to address your latest feedback. I can squash and clean-up the commits before merging but I didn't want to do it yet because it makes it harder to review just the new changes. |
|
Awesome @vially ! Announced it as well: https://twitter.com/AlexOkrushko/status/1262202129772621824 |
This pull-request adds an option to reset the state of the
retryBackoffoperator on successful emissions on the source observable.When this option is enabled (it's
offby default to maintain backwards compatibility) both the delay interval and the attempt counter gets reset when the source observable emits a value.Use cases
Some of the arguments and motivation for this option were nicely laid out in ReactiveX/rxjs#1413. A similar option was recently added to the retry operator (ReactiveX/rxjs#5280).
Quoting @benlesh from the linked issue:
Code review notes
The first four commits are not related to this pull-request and I can submit them separately if needed. They are related to some small nuisances I encountered when running the tests locally.