Skip to content
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

unexpected behaviour of R.once if the wrapped function throws an error. #3093

Open
adrian-gierakowski opened this issue Sep 30, 2020 · 7 comments

Comments

@adrian-gierakowski
Copy link

Currently, if during first call the wrapped function throws an error and the error is handled, each subsequent call will return undefined.

This is due to the fact that called is set to true before the function is called:

ramda/source/once.js

Lines 31 to 32 in ed191e6

called = true;
result = fn.apply(this, arguments);

Consider this example:

const Promise = require('aigle')

const wrappedFunc = () => {
  // Some code which due to a bug throws an error
}

const onceWrappedFunc = R.once(wrappedFunc)

P.retry({ times: 4 }, () => onceWrappedFunc())
  .then(result => console.log('result', result))
  .catch(error => console.log('error', error))

We are going to end up with .then callback being called with undefined instead catch callback being called with an error.

I think the most reasonable behaviour would be for R.once to store the error which happens during the first call and re-throw it on each subsequent call.

An alternative would be to swap the two lines of code quoted at the top of this message, but that would cause the wrappedFunc to be called multiple times, which breaks the expectation that it should only be ever called once.

@Bradcomp
Copy link
Member

I definitely think it's right for called to be set regardless of whether the function succeeds.

I can see the benefit for capturing and rethrowing the error each time. Would you be interested in opening a PR?

@adrian-gierakowski
Copy link
Author

@Bradcomp sure, I’ll submit a PR tomorrow.

@CrossEye
Copy link
Member

@adrian-gierakowski: Are you still interested in creating a PR?

@adrian-gierakowski
Copy link
Author

Yes, I’ve got an implementation ready.

@CrossEye
Copy link
Member

Great! Feel free to post a PR with it. We're trying to clean up all sorts of old issues on the way to getting v1.0 out the door.

@adrian-gierakowski
Copy link
Author

Oh, v1 finally. Nice!

@CrossEye
Copy link
Member

We're trying... finally!

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

No branches or pull requests

4 participants