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

Improve .retry() method functionality with delays and prevent calling on successful request #1527

Merged
merged 3 commits into from
Jul 17, 2020

Conversation

IAMtheIAM
Copy link
Contributor

@IAMtheIAM IAMtheIAM commented Nov 10, 2019

This PR adds new functionality:

  • The ability to specify the callback of .retry() as an async function. The purpose is to enable awaiting a Promise within the callback. This could be to used to fetch some external data, or by simply utilizing a setTimeout to resolve the promise after a specified delay. The result is that the .retry() is delayed by the given amount (or after the data is received). It does not break existing functionality i.e. the .retry() method can still be a normal synchronous function without any difference in behavior.

This PR also Resolves #1526

  • It prevents the .retry() method from being invoked on a successful request, and only allows it to invoke when there is a specified non-null error passed to .callback()

@IAMtheIAM
Copy link
Contributor Author

IAMtheIAM commented Nov 11, 2019

I'm not understanding why the build is failing, can someone explain?
The tests are not setup for us to run locally. There is no localhost:5000 so I cannot run any tests.

I've spent half a day trying to get the tests to pass but cannot. So I need some help. The code does work as far as I've manually tested.

@niftylettuce
Copy link
Collaborator

@IAMtheIAM it seems that the tests fail due to unhandled promise rejection related to retry tests https://travis-ci.org/visionmedia/superagent/jobs/610131622?utm_medium=notification&utm_source=github_status

if you fix + also fix merge conflicts (perhaps rebase to latest in our master branch and then add your work on top is best approach) - I would def merge and release this fix

@niftylettuce
Copy link
Collaborator

Closing until you submit a better PR

@niftylettuce
Copy link
Collaborator

Re-opened

@niftylettuce
Copy link
Collaborator

ping @3imed-jaberi

…ow easily delaying retries by using a Promise and setTimeout.
- Make ESLint happy
- Add babel plugin so aync/await works on node 6.x
@IAMtheIAM
Copy link
Contributor Author

IAMtheIAM commented Jul 17, 2020

@niftylettuce I resolved the merge conflicts by rebasing master onto my branch
I'm not sure about resolving the test yet, I'll take a look.

Update: The build failed with some lock file issue. Do you know what it means?

@niftylettuce niftylettuce merged commit 62eae78 into ladjs:master Jul 17, 2020
@niftylettuce
Copy link
Collaborator

Yes you forgot to update yarn lockfile and also you were missing adding @babel/plugin-transform-runtime to dev dependencies

@niftylettuce
Copy link
Collaborator

One moment, I'm fixing

@niftylettuce
Copy link
Collaborator

@IAMtheIAM can you please pull the latest from master branch? I pushed commit 0c4f96f but the tests are still failing. Please resolve and let me know.

@niftylettuce
Copy link
Collaborator

@niftylettuce
Copy link
Collaborator

I have reverted 62eae78 until tests are passing and this PR is re-submitted.

niftylettuce added a commit that referenced this pull request Jul 25, 2020
… calling on successful request (#1527)"

This reverts commit 62eae78.
@niftylettuce
Copy link
Collaborator

I'm going to leave the revert as is. This would be a major breaking change, as some may be using the retry callback to retry the request completely, even if no error occurred. Albeit it should be intended that a retry only happens when an error occurs, however we should leave this up to the user to override. If you do not want anything that has an error to even hit the shouldRetry function, then you could make a custom retry function callback.

@niftylettuce
Copy link
Collaborator

With regards to the async/promise support, that would be nice to have, but it seems your PR broke all the tests. If you can fix this and then re-submit (taking out the err !== null portion, then let me know.

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

Successfully merging this pull request may close these issues.

.retry() gets invoked even upon successful request
2 participants