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

retryOn, retryDelay, enhancements, and bugfixes/docs updates #218

Merged
merged 29 commits into from
Apr 2, 2020

Conversation

alex-cory
Copy link
Collaborator

@alex-cory alex-cory commented Mar 26, 2020

Notes

Proposed Solution

if retries > 0 then by default always retry if error or if response.status >= 300
if retryOn: [400] and retries > 0 then only retry on 400

Todos

  • fix types
  • fix lint errors
  • fix existing tests
  • clean up TODOs in code
  • tests
    • create tests for if we set timeout and retryDelay
    • retryDelay: 1000
    • retryDelay()
    • retryOn: [300]
    • retryOn()
    • retrying with no network (aka instead of going into the try retry we go into the catch retry)
    • when retryDelay is not a function or number
      • 🚫 there's an issue with this^ and I can't test if the function returns a number currently. We can however most likely test for a console.error and verify that.
    • when retryOn is not an array of http status codes or a function
    • should not be content-type: application/json by default if using FormData for request body
    • console.warn that says something like To have a retryDelay set, you need to have retries > 0
    • invariant retries >= 0 and typeof retries === 'number'
    • double check if we have a test for fail 1st, retry and succeed test
  • error handling
    • when retryDelay is not a function returning a number or number
    • when retryOn is not an array of http status codes or a function
  • somehow reset attempts after completing the last retry
  • re-order all options in docs alphabetically
  • codesandbox examples with retryOn and retryDelay
  • better documentation for retryOn and retryDelay. Take a look at fetch-retry docs
  • make sure all the defaults are the correct values in the docs. Also update documentation in table for retryOn and retryDelay to explain how we default to 3 retries if there's network errors with 1000ms as the retryDelay
  • difference between timeout and retryDelay. WIP
    • timeout will cancel the current pending http request if it's not completed before this time is up
      request --------- > abort ------------> retry request
      |------ timeout ------|
      
    • retryDelay the time after a failed http request and when to retry the new one
      request ----------> error ---------------> retry request
                            |---- retryDelay----|
      
    • Mental model if you're using both
      initial request ----- > error ------------> retry ---------> success or error
      | ----- timeout -------- | --- retryDelay --- | --- timeout --- | 
      
    • Questions:
      1. What happens if we abort before completing the 1st request out of 3 (2 retries). Do we retry 2 more times after aborting?
      const request = useFetch('url', { retries: 2 })
      const handleMakeRequest = async () => {
        // if we cancel/abort this immediately, should it retry?
        const todo = await request.post('/todos', { no: 'way' })
      }
      <button onClick={handleMakeRequest}>Make Request</button>
      <button onClick={request.abort}>Abort</button>      
      A: it would just cancel/abort the 1st request and proceed to retry the other 2 times. We could potentially have an option to cancel them all.

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2020

This pull request introduces 1 alert when merging 3d0cfca into 1d42e23 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2020

This pull request introduces 1 alert when merging 1ed2be7 into 1d42e23 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@alex-cory alex-cory marked this pull request as ready for review April 1, 2020 22:51
Alex Cory added 2 commits April 1, 2020 16:53
@alex-cory alex-cory changed the title retryOn, retryDelay, and small bugfixes/docs updates retryOn, retryDelay, enhancements, and bugfixes/docs updates Apr 2, 2020
@alex-cory alex-cory merged commit dc51a46 into master Apr 2, 2020
@alex-cory alex-cory deleted the retryOn branch April 2, 2020 01:07
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.

None yet

1 participant