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

Bacon.retry with convenience friends #310

Merged
merged 5 commits into from May 14, 2014

Conversation

Projects
None yet
3 participants
@mileskin
Contributor

mileskin commented Jan 10, 2014

We've had these for several months now in the project I'm working on, and the API is quite settled. Especially with ajax requests it's really handy to be able to retry conditionally. However, Bacon.retry is by no means ajax specific.

Example usage with jQuery ajax:

Bacon.retry
  source: -> Bacon.fromPromise($.ajax(...))
  retries: 4
  interval: ({retriesDone}) -> (1 + retriesDone) * 5000
  isRetryable: (response) -> response.status isnt 401

So what do you guys think, would it be generic enough to have this in the core? I would love to hear your feedback and would be happy to change the code if necessary.

One thing concerning the spec: is there a reason why error content can't be asserted at the moment, e.g. [1, 2, error(foo: 'bar')]? I tried changing .toValue in SpecHelper but only got a bunch of failing tests. I think it would be neat to have assertable errors whether this PR gets merged or not. Any thoughts?

@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin Jan 10, 2014

Contributor

Doc needs to be updated obviously, I didn't bother until I hear your comments.

Contributor

mileskin commented Jan 10, 2014

Doc needs to be updated obviously, I didn't bother until I hear your comments.

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Jan 10, 2014

Contributor

This is good stuff. The question we have to deal with now is where do we put all this goodness.

Contributor

raimohanska commented Jan 10, 2014

This is good stuff. The question we have to deal with now is where do we put all this goodness.

@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin Jan 10, 2014

Contributor

Now allows the client code to calculate interval by context: number of retries done / whether the retry is being issued due to an error or an invalid value. Kudos to @jonifreeman for suggesting this.

Contributor

mileskin commented Jan 10, 2014

Now allows the client code to calculate interval by context: number of retries done / whether the retry is being issued due to an error or an invalid value. Kudos to @jonifreeman for suggesting this.

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Jan 10, 2014

Contributor

Do you have an opinion on #311? I'd be glad to include this but not willing to increase the core size. Should we make this an optional feature?

Contributor

raimohanska commented Jan 10, 2014

Do you have an opinion on #311? I'd be glad to include this but not willing to increase the core size. Should we make this an optional feature?

@jonifreeman

This comment has been minimized.

Show comment
Hide comment
@jonifreeman

jonifreeman Jan 10, 2014

Contributor

Current implementation bolts 'isValidValue' check to 'retry' combinator which in my opinion should not be there. Taking that away same effect can be implemented by transforming the source stream:

 Bacon.retry(source: someSource.flatMap(function(x) { return x == "FOO" ? Bacon.error("invalid value") : Bacon.once(x) }))
Contributor

jonifreeman commented Jan 10, 2014

Current implementation bolts 'isValidValue' check to 'retry' combinator which in my opinion should not be there. Taking that away same effect can be implemented by transforming the source stream:

 Bacon.retry(source: someSource.flatMap(function(x) { return x == "FOO" ? Bacon.error("invalid value") : Bacon.once(x) }))
@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin Jan 10, 2014

Contributor

@jonifreeman I'm fine stripping isValidValue away if people don't like it. Someone else willing to comment on the subject?

Contributor

mileskin commented Jan 10, 2014

@jonifreeman I'm fine stripping isValidValue away if people don't like it. Someone else willing to comment on the subject?

@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin Jan 10, 2014

Contributor

IMO it's a nice (though nice to have) abstraction and a "pair" for isRetryable but I certainly understand that it's not an essential part of retry in a way isRetryable is.

Contributor

mileskin commented Jan 10, 2014

IMO it's a nice (though nice to have) abstraction and a "pair" for isRetryable but I certainly understand that it's not an essential part of retry in a way isRetryable is.

@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin Jan 10, 2014

Contributor

@raimohanska about #311 I'm totally fine not having retry in core but instead in "extras" or as a plugin. Unfortunately I probably don't have time to boost any of the suggested solutions in the near future myself... :sad_panda

Contributor

mileskin commented Jan 10, 2014

@raimohanska about #311 I'm totally fine not having retry in core but instead in "extras" or as a plugin. Unfortunately I probably don't have time to boost any of the suggested solutions in the near future myself... :sad_panda

@jonifreeman

This comment has been minimized.

Show comment
Hide comment
@jonifreeman

jonifreeman Jan 10, 2014

Contributor

@mileskin Core combinators should be minimal, composable and orthogonal. It may look like a nice to have convenience but in my opinion it adds complexity.

Programmer1: "Hey, what is this 'retry' thing?"

Programmer2: "It just retries when the source stream emits Errors."

Now, any seasoned Bacon hacker already knows how to retry on invalid values, by transforming the source stream to emit Errors on invalid values.

This is simpler to understand than:

Programmer2: "It just retries when the source stream emits Errors, or if you provide a custom predicate function for values."

It adds complexity since eventually both ways are used. Consider for instance the following scenario.

Bacon.retry({ source: foo, isValidValue: function(x) { return x == 30 }})

Then the UI spec changes. Valid value is no longer a constant but a time varying value (like the age of a selected person). That convenience method no longer works. The poor programmer must rewire his brain and realize that it can be implemented by removing 'isValidValue' and transforming the source stream to emit Errors on sampled stream.

It may look like a small issue here but in a bigger context these things stack up. If there's suddenly many combinators with overlapping features then it inevitably follows that different styles are used in a same codebase. I'm not saying that choice is bad but here it is unneccesary.

Contributor

jonifreeman commented Jan 10, 2014

@mileskin Core combinators should be minimal, composable and orthogonal. It may look like a nice to have convenience but in my opinion it adds complexity.

Programmer1: "Hey, what is this 'retry' thing?"

Programmer2: "It just retries when the source stream emits Errors."

Now, any seasoned Bacon hacker already knows how to retry on invalid values, by transforming the source stream to emit Errors on invalid values.

This is simpler to understand than:

Programmer2: "It just retries when the source stream emits Errors, or if you provide a custom predicate function for values."

It adds complexity since eventually both ways are used. Consider for instance the following scenario.

Bacon.retry({ source: foo, isValidValue: function(x) { return x == 30 }})

Then the UI spec changes. Valid value is no longer a constant but a time varying value (like the age of a selected person). That convenience method no longer works. The poor programmer must rewire his brain and realize that it can be implemented by removing 'isValidValue' and transforming the source stream to emit Errors on sampled stream.

It may look like a small issue here but in a bigger context these things stack up. If there's suddenly many combinators with overlapping features then it inevitably follows that different styles are used in a same codebase. I'm not saying that choice is bad but here it is unneccesary.

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Jan 10, 2014

Contributor

I guess I agree with @jonifreeman on this. It's better to use Error objects to represent (retryable) errors.

Do you feel that producing Errors is somewhat harder that producing values representing Errors? Do we need combinators to make Errors easier to produce?

Contributor

raimohanska commented Jan 10, 2014

I guess I agree with @jonifreeman on this. It's better to use Error objects to represent (retryable) errors.

Do you feel that producing Errors is somewhat harder that producing values representing Errors? Do we need combinators to make Errors easier to produce?

@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin Jan 10, 2014

Contributor

@jonifreeman @raimohanska Fair enough, I'll remove all things isValidValue and commit to this PR. Thanks for the good argumentation.

Contributor

mileskin commented Jan 10, 2014

@jonifreeman @raimohanska Fair enough, I'll remove all things isValidValue and commit to this PR. Thanks for the good argumentation.

Remove everything related to isValidValue from Bacon.retry
Similar effect can (and should) be implemented in the client code,
by transforming the source stream as suggested by @jonifreeman:

Bacon.retry({source: someSource.flatMap(function(x) { return x === "FOO" ? Bacon.error("invalid value") : Bacon.once(x) })})

@raimohanska raimohanska merged commit 519ea2f into baconjs:master May 14, 2014

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska May 14, 2014

Contributor

Merged! I'm working on backlog now :)

TODO: update readme-src.coffee

Contributor

raimohanska commented May 14, 2014

Merged! I'm working on backlog now :)

TODO: update readme-src.coffee

@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin May 14, 2014

Contributor

🍻 !

Contributor

mileskin commented May 14, 2014

🍻 !

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska May 14, 2014

Contributor

are you up for a little readme update? :)

I removed Bacon.error as duplicate in 115eedd, so there's flatMapError and Bacon.retry to do.

Contributor

raimohanska commented May 14, 2014

are you up for a little readme update? :)

I removed Bacon.error as duplicate in 115eedd, so there's flatMapError and Bacon.retry to do.

@mileskin

This comment has been minimized.

Show comment
Hide comment
@mileskin

mileskin May 15, 2014

Contributor

Sure, I'll try to arrange some time asap.

Contributor

mileskin commented May 15, 2014

Sure, I'll try to arrange some time asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment